Includes of system headers are never implicitly relative to the source file

[bavison]

Sometimes, #include directives with angle-bracket filespec delimiters are used (or abused) to defeat the preprocessor's behaviour where it tries to find a header file at a path relative to the file containing the directive.

Without this fix, any non-root header file, foo/bar.h, which does

  #include <bar.h>

while intending to include a root-level header file, will instead enter an infinite inclusion loop, terminating when the inclusion stack overflows with a #include nested too deeply error.

See also this cppcheck issue.

[danmar]

is it possible to write some test for this? Something similar to this maybe:

static void nestedInclude()

[bavison]

Hope this will do - the test detects the difference between the old and new simplecpp behaviour. I found I had to create an empty limits.h file in the filedata map, since I guess simplecpp::preprocess doesn't know that's a standard header.

[firewave]

This indicates that code above which also checks op for < might never be executed,

[bavison]

Is inctok->op supposed to have any meaning here? By dumping out its value every time we reach this point in a run of the testrunner binary, I can see it's a null character in every case except in include7() and include8(), which are both testing the case where the argument to #include is derived by expanding a preprocessor macro; in these cases it holds a < character. So perhaps it's left over from the macro expansion? (Apologies, I'm new to the cppcheck innards, I'm not familiar with how it works.)

By chance, until I added my new test, these were also the only cases where the filespec in inctok->str() used angle brackets, so for the cases tested by testrunner so far, testing inctok->op was adequate. I agree this looks wrong, but I'm unsure whether the fix should be to look at inctok->str() instead, or whether to fix the initialisation of inctok->op.

[firewave]

My initial observation was based on a short look and was wrong. I am also not familiar in how it works.

op is set to < in that code above. Since with your change that is not used anymore that can be removed.

[firewave]

An additional note. It seems op should only be set if it is a single character token. And if the token isn't already something like a name, number or comment. That wasn't done consistently though - I addressed that within #285.

[firewave]

This is scary. I finally got around to checking why the issue I introduced in #276 and fixed in #281 did not trigger any test failures. And it turns out it is caused by the same code you just fixed.

simplecpp is completely void of tests with additional files at the moment. That has been a source of discussion in #261.

There's also tests lacking in cppcheck which I have partially added locally.

[danmar]

hmm.. I feel I can merge this. However when I try it locally it does not seem to work.

I have this code in my file 1.c:

#include <1.h>

And 1.h contains:

int x;

when I run simplecpp with command simplecpp 1.c it includes the 1.h file.

It works as it should for you?

[firewave]

hmm.. I feel I can merge this. However when I try it locally it does not seem to work.

That's why we need unit tests with headers/includes...

[danmar]

The title of this PR says that "system headers are never relative to the source file". That is not true.

See:

$ gcc -E 1.c
1.c:1:10: fatal error: 1.h: No such file or directory

$ gcc -E -I. 1.c
=> 1.h is included.

[bavison]

when I run simplecpp with command simplecpp 1.c it includes the 1.h file.

That looks to have been caused by openHeader() also attempting to find a system header at a path relative to the source file. My latest push should have fixed that. Unfortunately, I haven't managed to work out how to make the unit test framework detect this change, but running the simplecpp binary on your 1.c now has the desired behaviour (it fails unless you explicitly use -I.).

The title of this PR says that "system headers are never relative to the source file". That is not true.

OK, I have added the word "implicitly" into the commit summary, hope this is sufficient.

For what it's worth, I found the following description in the gcc man page (I realise this isn't unversal to all compilers):

           The lookup order is as follows:

           1.  For the quote form of the include directive, the directory of
               the current file is searched first.

           2.  For the quote form of the include directive, the directories
               specified by -iquote options are searched in left-to-right
               order, as they appear on the command line.

           3.  Directories specified with -I options are scanned in left-to-
               right order.

           4.  Directories specified with -isystem options are scanned in
               left-to-right order.

           5.  Standard system directories are scanned.

           6.  Directories specified with -idirafter options are scanned in
               left-to-right order.

[firewave]

Unfortunately, I haven't managed to work out how to make the unit test framework detect this change

I will add such tests to Cppcheck soon with the referenced PR.

[danmar]

Thanks I think this looks good. If CI is happy I'll merge it. Let's consider the testing later separately. I also think we could explore using pytest in simplecpp.