Re: [PATCH] D53354: [WIP][NOT FOR COMMIT][PROTOTYPE] clang-scan-deps: dependency scanning tool rough prototype
Hi Alex, On Fri, Oct 19, 2018 at 1:11 AM Alex Lorenz via Phabricator via cfe-commits wrote: > > > Have you checked the tool //Include What You Use//? I'm curious in what way > > the mishappenings of that tool present themselves in yours. There were some > > challenges not overcome in a good way in IWYU, their readme goes into a bit > > more detail on this. > > I haven't looked into IWYU that much. Could you please elaborate on which > mishappenings you think might present themselves here? IWYU maintainer here. I'm not sure exactly what your tool wants to do, or what Whisperity meant, but we have a couple of doc pages on why IWYU is tricky: https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/WhatIsAUse.md https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/WhyIWYUIsDifficult.md FWIW, - Kim ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53354: [WIP][NOT FOR COMMIT][PROTOTYPE] clang-scan-deps: dependency scanning tool rough prototype
dexonsmith added inline comments.
Comment at: lib/Lex/FilterToIncludes.cpp:628
+ First = Id.Last;
+ auto Kind = llvm::StringSwitch(Id.Name)
+ .Case("include", pp_include)
arphaman wrote:
> arphaman wrote:
> > arphaman wrote:
> > > ddunbar wrote:
> > > > What is our feeling w.r.t. _Pragma, which can in theory influence the
> > > > preprocessor. I'm not sure this model can sanely support it?
> > > We need to look into that. In the worst case we can always avoid
> > > minimizing the file if it has a _Pragma anywhere in it.
> > We also have to handle cases like this one:
> >
> > foo.h:
> > ```
> > #define PP _Pragma
> > ```
> >
> > bar.h:
> > ```
> > PP ("foo")
> > ```
> >
> > Unfortunately this can't be handled by just disabling minimization for
> > `foo.h` as PP will be stripped out from `bar.h`. Furthermore, this pattern
> > is quite reasonable, so we should expect it in the code we receive. Right
> > now I can't think of a reasonable solution for this problem.
> >
> > There's also a more "evil" case:
> >
> > ```
> > #define P(A, B) A##B
> >
> > P(_Pra,gma)("foo")
> > ```
> >
> > It would be reasonable to introduce a warning for the use of `_Pragma`
> > token that was created using PP token concatenation and just hope that our
> > users won't use this pattern.
> One possible solution to the first issue is to simply fallback to the regular
> -Eonly invocation if we detect an include of a file that has a macro with a
> `_Pragma` in it. Then we could emit a remark to the user saying that their
> build could be faster if they rewrite their code so that this pattern no
> longer occurs.
Hmm. Our plan for `@import` is to stop supporting code such as:
```
#define IMPORT(M) @import M
IMPORT(LLVMSupport);
```
where the @import is buried. I.e., start erroring out in normal Clang builds.
Perhaps it would be reasonable to similarly disallow `_Pragma` usage that
buries import/include statements. I.e., do not support (even in normal builds)
the following:
```
#define IMPORT(M) _Pragma("clang module import " #M)
IMPORT(LLVMSupport)
```
Possibly, this could be a warning that is error-by-default.
Repository:
rC Clang
https://reviews.llvm.org/D53354
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53354: [WIP][NOT FOR COMMIT][PROTOTYPE] clang-scan-deps: dependency scanning tool rough prototype
arphaman added a comment. In https://reviews.llvm.org/D53354#1267376, @whisperity wrote: > With regards to https://reviews.llvm.org/D53352: you can change the diff > related to a patch whilst keeping discuccion and metadata of the diff. Good point, thanks! > Please add a generic description to the diff for an easy skimming on what you > are accomplishing. I added the description, thanks. > If I get this right, your tool will spit out a CPP file that is only include > directives and perhaps the related conditional logic, or the final output of > your tool is a file list? It's both, as there are two tools in this patch. The first is the `clang-filter-to-includes` tool, which is wrapper around our source minimization optimization (i.e. it spits out a source with `#includes` and other PP directives). The `clang-scan-deps` tool integrates this optimization into a service-like tool that will produce a set of dependencies for a set of compilation commands. Right now it's mainly a benchmark that compares the speed of the fast scanner to the regular preprocessor invocation. > This is different than the `-M` flags in a way that it keeps conditions sane, > rather than spitting out what includes were used if the input, with regards > to the compiler options, was to be compiled? It's supposed to produce identical output to the run of `-Eonly` with the `-MD` flag. Right now we don't know how we want to expose the dependency set to the clients. > Have you checked the tool //Include What You Use//? I'm curious in what way > the mishappenings of that tool present themselves in yours. There were some > challenges not overcome in a good way in IWYU, their readme goes into a bit > more detail on this. I haven't looked into IWYU that much. Could you please elaborate on which mishappenings you think might present themselves here? Repository: rC Clang https://reviews.llvm.org/D53354 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53354: [WIP][NOT FOR COMMIT][PROTOTYPE] clang-scan-deps: dependency scanning tool rough prototype
arphaman added inline comments.
Comment at: lib/Lex/FilterToIncludes.cpp:628
+ First = Id.Last;
+ auto Kind = llvm::StringSwitch(Id.Name)
+ .Case("include", pp_include)
arphaman wrote:
> arphaman wrote:
> > ddunbar wrote:
> > > What is our feeling w.r.t. _Pragma, which can in theory influence the
> > > preprocessor. I'm not sure this model can sanely support it?
> > We need to look into that. In the worst case we can always avoid minimizing
> > the file if it has a _Pragma anywhere in it.
> We also have to handle cases like this one:
>
> foo.h:
> ```
> #define PP _Pragma
> ```
>
> bar.h:
> ```
> PP ("foo")
> ```
>
> Unfortunately this can't be handled by just disabling minimization for
> `foo.h` as PP will be stripped out from `bar.h`. Furthermore, this pattern is
> quite reasonable, so we should expect it in the code we receive. Right now I
> can't think of a reasonable solution for this problem.
>
> There's also a more "evil" case:
>
> ```
> #define P(A, B) A##B
>
> P(_Pra,gma)("foo")
> ```
>
> It would be reasonable to introduce a warning for the use of `_Pragma` token
> that was created using PP token concatenation and just hope that our users
> won't use this pattern.
One possible solution to the first issue is to simply fallback to the regular
-Eonly invocation if we detect an include of a file that has a macro with a
`_Pragma` in it. Then we could emit a remark to the user saying that their
build could be faster if they rewrite their code so that this pattern no longer
occurs.
Repository:
rC Clang
https://reviews.llvm.org/D53354
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53354: [WIP][NOT FOR COMMIT][PROTOTYPE] clang-scan-deps: dependency scanning tool rough prototype
arphaman added inline comments.
Comment at: lib/Lex/FilterToIncludes.cpp:628
+ First = Id.Last;
+ auto Kind = llvm::StringSwitch(Id.Name)
+ .Case("include", pp_include)
arphaman wrote:
> ddunbar wrote:
> > What is our feeling w.r.t. _Pragma, which can in theory influence the
> > preprocessor. I'm not sure this model can sanely support it?
> We need to look into that. In the worst case we can always avoid minimizing
> the file if it has a _Pragma anywhere in it.
We also have to handle cases like this one:
foo.h:
```
#define PP _Pragma
```
bar.h:
```
PP ("foo")
```
Unfortunately this can't be handled by just disabling minimization for `foo.h`
as PP will be stripped out from `bar.h`. Furthermore, this pattern is quite
reasonable, so we should expect it in the code we receive. Right now I can't
think of a reasonable solution for this problem.
There's also a more "evil" case:
```
#define P(A, B) A##B
P(_Pra,gma)("foo")
```
It would be reasonable to introduce a warning for the use of `_Pragma` token
that was created using PP token concatenation and just hope that our users
won't use this pattern.
Repository:
rC Clang
https://reviews.llvm.org/D53354
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53354: [WIP][NOT FOR COMMIT][PROTOTYPE] clang-scan-deps: dependency scanning tool rough prototype
arphaman added inline comments.
Comment at: lib/Lex/FilterToIncludes.cpp:628
+ First = Id.Last;
+ auto Kind = llvm::StringSwitch(Id.Name)
+ .Case("include", pp_include)
ddunbar wrote:
> What is our feeling w.r.t. _Pragma, which can in theory influence the
> preprocessor. I'm not sure this model can sanely support it?
We need to look into that. In the worst case we can always avoid minimizing the
file if it has a _Pragma anywhere in it.
Repository:
rC Clang
https://reviews.llvm.org/D53354
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53354: [WIP][NOT FOR COMMIT][PROTOTYPE] clang-scan-deps: dependency scanning tool rough prototype
ddunbar added inline comments.
Comment at: lib/Lex/FilterToIncludes.cpp:628
+ First = Id.Last;
+ auto Kind = llvm::StringSwitch(Id.Name)
+ .Case("include", pp_include)
What is our feeling w.r.t. _Pragma, which can in theory influence the
preprocessor. I'm not sure this model can sanely support it?
Repository:
rC Clang
https://reviews.llvm.org/D53354
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53354: [WIP][NOT FOR COMMIT][PROTOTYPE] clang-scan-deps: dependency scanning tool rough prototype
whisperity added a comment. With regards to https://reviews.llvm.org/D53352: you can change the diff related to a patch whilst keeping discuccion and metadata of the diff. Please add a generic description to the diff for an easy skimming on what you are accomplishing. If I get this right, your tool will spit out a CPP file that is only include directives and perhaps the related conditional logic, or the final output of your tool is a file list? This is different than the `-M` flags in a way that it keeps conditions sane, rather than spitting out what includes were used if the input, with regards to the compiler options, was to be compiled? Have you checked the tool //Include What You Use//? I'm curious in what way the mishappenings of that tool present themselves in yours. There were some challenges not overcome in a good way in IWYU, their readme goes into a bit more detail on this. Repository: rC Clang https://reviews.llvm.org/D53354 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
