[vlc-devel] [PATCH 3/4] core: fix header quoting/path consistency

Lyndon Brown jnqnfe at gmail.com
Fri Oct 2 23:11:15 CEST 2020


On Fri, 2020-10-02 at 10:26 +0200, Thomas Guillem wrote:
> >  - inconsistency of <libvlc.h> vs. "libvlc.h" vs. "../libvlc.h"
> 
> OK, <libvlc.h> is wrong, but I prefer  "libvlc.h" than "../libvlc.h.
> Otherwise, what's the use of "-I" ?

I don't believe using <libvlc.h> is wrong if your intention is to rely
upon an "-I" path to find it, quite the opposite, it's "libvlc.h"
that's wrong. While the difference is implementation defined, my
understanding is fundamentally that use of quotes means first check the
same directory as the file doing the include, before moving on to
checking the same "-I" paths checked by the angle bracket search. Thus
in any case where we're relying upon an "-I" path, we should really be
using angle brackets to skip the then pointless initial same-directory
search.

So if "../libvlc.h" is disliked then it should actually be <libvlc.h>
that we want rather than "libvlc.h". The same applies to headers in
sibling src/ subdirectories; we should use <foo/bar.h> rather than
"foo/bar.h" to skip the pointless initial same-directory search.

One benefit of "../libvlc.h", even if it's ever so slightly uglier than
"libvlc.h" or <libvlc.h> is that this immediately points to the right
file, avoiding unnecessary further path searching. We have no need for
such path searching with libvlc.h or private core headers located in
sibling directories. We can locate them immediately simply with a '../'
and quotes. As quick as it may be, why make the build system issue
hundreds/thousands of unnecessary FS searches just for such an
unimportant and tiny aesthetic detail. "libvlc.h" is the very worst in
terms of efficiency.

"-I" serves a very useful purpose, particularly for paths that exist
outside of the project, and I also feel it okay for locating the public
headers in /include. Avoiding an "-I" for <source>/src doesn't make "-
I" pointless, and besides, it doesn't follow that just because "-I"
exists that you have to use it.

Furthermore things are just more straightforward, clear and obvious to
anyone looking at the code if you directly refer to the src/* private
headers with relative paths than unintuitive (imo) and obscure quoted
paths relative to a non-obvious "-I" <source>/src, helping avoid the
very confusion that it caused me.

It is not obvious what set of "-I" paths are actually in play for each
object given the mass of build system code and poor and complex
autotools documentation. I had to do a bunch of experimentation to
quickly get a sufficient handle on it, and made a significant mistake
in the previous 'platform grouping' submission as a result of
confusion. I wouldn't have needed to experiment and wouldn't have made
the mistake, if things were straightforward like this from the start.

I think we should remove the "-I" for <source>/src.

> > -#include "config/configuration.h"
> > +#include "../config/configuration.h"
> 
> Again, I prefer #include "config/configuration.h"

As above, that involves both a pointless initial same-directory search,
then searching "-I" locations one by one. It is more efficient if
switched to <config/configuration.h>, avoiding the first check, but
still inefficient, compared to just directly finding the file with
"../config/configuration.h".

I agree that the '../' prefix isn't the most aesthetically pleasing,
but on balance I'd much prefer the efficiency and simplicity of the
direct reference, than trade that only for the purpose of not having to
see the '../'.

> Not really fan of this patch, that change many files for no specific
> gain.

The primary specific gain is fixing consistency, which helps in terms
of clarity and such. To me, improving clarity/consistency here is
worthwhile.

The particular form of include preferred here is the most efficient
choice, and my personal preferred choice, but whichever form consensus
leans towards, there is a lot of inconsistency to fix all over, and
thus a lot of files to fix, if we can agree on fixing inconsistency.



More information about the vlc-devel mailing list