New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

false diagnostics in include files due to never-firing #ifdefs #115

Closed
cmm opened this Issue Nov 10, 2018 · 4 comments

Comments

Projects
None yet
3 participants
@cmm
Copy link

cmm commented Nov 10, 2018

Makefile :

c.o: c.c; gcc -o $@ -c $<

c.c :

#define PLEASE
#include "i.h"

i.h :

#ifndef PLEASE
#error say PLEASE
#endif

Run bear make to get a compile_commands.json, open i.h in an editor configured to work with lsp & ccls, see how the #error say PLEASE lights up (even though PLEASE is clearly defined in the only compilation that i.h participates in).

@MaskRay

This comment has been minimized.

Copy link
Owner

MaskRay commented Nov 11, 2018

I am aware of this issue. https://github.com/MaskRay/ccls/releases/tag/0.20180924 It parsed import file (foo.c) to get completion/diagnostics when you are editing foo.h. I soon gave up that idea as the lack of preamble optimization made completion very slow

Added a section to https://github.com/MaskRay/ccls/wiki/FAQ

Diagnostics in headers
For efficiency, headers are compiled on their own (as if clang [options] a.h) to utilize Clang's preamble optimization.

For headers that are not self-contained (e.g. some /usr/include/bits/*.h which require a predefined macro) ccls emits diagnostics that you don't see when building the project.

// a.c
#define M
#include "a.h"

// a.h
#ifndef M
#error M not defined
#endif

One approach ccls explored before was to compile its import file a.c, but it caused severe performance issue for some projects. For smaller files this does not matter and checking for a header guard may help the situation.

BTW (see FAQ), if you don't need any cflags, neither .ccls nor compile_commands.json is required.

@madscientist

This comment has been minimized.

Copy link

madscientist commented Dec 20, 2018

Thanks @MaskRay I read the FAQ which does summarize the problem well. Just to be clear, what the FAQ is saying is that it's a known limitation at this time?

What I would like to see (I know this is probably not trivial) would be (a) allowing for both .ccls and compile_commands.json to be used at the same time, so that .ccls can augment the content of compile_commands.json. Or, maybe a separate .ccls-extras file or something that would apply to both if that would be simpler.

Then the .ccls syntax could be extended to provide a %h or similar that would have extra flags applied when headers are being compiled on their own. Also it would be helpful of course to allow extra flags to be applied via %c and %cpp to augment the content of compile_commands.json, such as --sysroot etc. that I've needed (described in other issues).

Finally, it would be very nice if the .ccls syntax could be enhanced to expand environment variables. I understand these would have to be set before ccls itself was invoked.

These enhancements would allow us to create a .ccls (or .ccls-extra or whatever) file which is checked in to Git as part of the project definition to set extra flags for this project, and also allow customizations (via environment variables set in init.el or similar) of paths, etc. on a per user basis.

@MaskRay MaskRay referenced this issue Dec 21, 2018

Merged

Extend .ccls #171

@MaskRay

This comment has been minimized.

Copy link
Owner

MaskRay commented Dec 21, 2018

Thanks @MaskRay I read the FAQ which does summarize the problem well. Just to be clear, what the FAQ is saying is that it's a known limitation at this time?

Yes.

What I would like to see (I know this is probably not trivial) would be (a) allowing for both .ccls and compile_commands.json to be used at the same time, so that .ccls can augment the content of compile_commands.json. Or, maybe a separate .ccls-extras file or something that would apply to both if that would be simpler.

This is a reasonable extension to the current behavior.

We've already have 3 things to describe cflags: .ccls, compile_commands.json, and initialization options. They just need a better collaboration (thinking of what comes first, what augments/overrides what).

Then the .ccls syntax could be extended to provide a %h or similar that would have extra flags applied when headers are being compiled on their own. Also it would be helpful of course to allow extra flags to be applied via %c and %cpp to augment the content of compile_commands.json, such as --sysroot etc. that I've needed (described in other issues).

This is also reasonable.

Finally, it would be very nice if the .ccls syntax could be enhanced to expand environment variables. I understand these would have to be set before ccls itself was invoked.

These enhancements would allow us to create a .ccls (or .ccls-extra or whatever) file which is checked in to Git as part of the project definition to set extra flags for this project, and also allow customizations (via environment variables set in init.el or similar) of paths, etc. on a per user basis.

#171

@madscientist

This comment has been minimized.

Copy link

madscientist commented Dec 21, 2018

We've already have 3 things to describe cflags: .ccls, compile_commands.json, and initialization options. They just need a better collaboration (thinking of what comes first, what augments/overrides what).

Thinking about this, I suggest this set of priorities, from lowest to highest:

  • compile_commands.json
  • .ccls
  • initialization options

My justification for this ordering is:

  1. compile_commands.json is typically auto-generated by other tools, which can make it difficult and annoying to customize so it should have the lowest priority
  2. .ccls will typically be checked into the source control repository, since it lives in the project directory, so it is good for general-purpose project fixups to compile_commands.json that all users would probably need.
  3. Initialization options are set by the individual user in their editor initialization, so these should have the highest priority.

Translating these general rules into concrete behavior is maybe not so simple. For non-preprocessor options like -O etc. the obvious thing is to start with the lowest priority flags and append the higher priority flags, since compilers take the last option (e.g., if you have -O3 -O1 then -O1 takes precedence).

However, for preprocessor flags it's the inverse: if you have -Ifoo -Ibar then headers found in foo will take precedence over headers found in bar (of the same name). The only real break with this rule is -D vs -U; e.g., -DFOO=1 -UFOO; you'd want the -U to come last. But maybe this is too obscure of a case to worry about.

Since ccls cares more about preprocessor options and not as much about other options, maybe it should say that higher-priority sources will be prepended to lower-priority sources. Of course there are other options ccls will care about I expect like architecture choosing options etc.

Another idea would be to have the user explicitly request things in the order they wanted.

For example, if the .ccls file understood a new option like %compile_commands.json then this could control where those flags were inserted.

So, in this situation the rules for ccls would be:

  1. If a .ccls file is found, then read it. If during the read we see %compile_commands.json, look for that file and if it's found read it and append any flags to what we have so far. If %compile_commands.json is not found then don't read that file, even if exists. Then finish reading.
  2. If no .ccls file is found, then look for compile_commands.json and if it's found use it. If not pretend we got a .ccls file containing %clang (as we do today).

Then .ccls could look something like:

%c %cpp
--sysroot=foo
%h %hpp
--include=bar.h
%compile_commands.json
%c %cpp
-O0
-g
%c
-std=gnu11
%cpp
-std=c++14

or something like that (allowing multiple %c etc. sections which are appended together).

One nice thing about this is it allows users to choose to ignore compile_commands.json, even if it exists, if they wanted to.

I'm not sure how initialization options fit into this. Maybe they're just added to the end. Or, maybe those options themselves have a way to request loading of the .ccls file contents at a particular spot. It would be nice if there was alignment between the .ccls file and the initialization options, so that they used a similar format and had the same capabilities. Maybe switch to .ccls.json file and define a JSON format that could be shared by that file and initialization options 😮 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment