dexonsmith added inline comments.
================ Comment at: clang/test/Preprocessor/header-search-user-entries.c:9-11 +// CHECK: remark: user-provided search path used: '{{.*}}/header-search-user-entries/a' +// CHECK: remark: user-provided search path used: '{{.*}}/header-search-user-entries/a_next' +// CHECK: remark: user-provided search path used: '{{.*}}/header-search-user-entries/d' ---------------- dexonsmith wrote: > dexonsmith wrote: > > jansvoboda11 wrote: > > > jansvoboda11 wrote: > > > > jansvoboda11 wrote: > > > > > dexonsmith wrote: > > > > > > If `-verify` doesn't work (hopefully it does), I think you'll need > > > > > > to add some `CHECK-NOT` / `CHECK-NEXT` / etc. or else you're not > > > > > > testing for the absence of other diagnostics. > > > > > > > > > > > > Please also test: > > > > > > - Framework search path (used vs. not used) > > > > > > - System search path (used vs. not used) > > > > > > - One search path described relative to `-isysroot` (used vs. not > > > > > > used) > > > > > > - Header map (matched and used vs. matched but not used vs. not > > > > > > matched -- for the middle case, I'm not sure what result we > > > > > > actually want) > > > > > > - A path only used via `#if __has_include()` (when it's not > > > > > > followed by an `#include` or `#import`) > > > > > > - A path only used via ObjC's `#import` > > > > > > > > > > > > If one of them doesn't work and it's complex enough to separate > > > > > > into a follow up, I think that'd be fine, but please find somewhere > > > > > > to leave a FIXME. > > > > > Good point, I'll work on improving the coverage. > > > > I tried using `-verify` together with `expected-remark-re` (to match > > > > **relative** paths) and `@*` (to match any/no line number), but those > > > > don't seem to work together. > > > > > > > > This expectation: > > > > ``` > > > > // expected-remark-re @* {{search path used: > > > > '{{.*}}/search-path-usage/FwA'}} > > > > ``` > > > > produces: > > > > ``` > > > > error: 'error' diagnostics seen but not expected: > > > > File > > > > /Users/Jan/Code/upstream-llvm/clang/test/Preprocessor/search-path-usage-headers.m > > > > Line 30: cannot find start ('{{') of expected regex > > > > error: 'remark' diagnostics seen but not expected: > > > > (frontend): search path used: > > > > '/Users/Jan/Code/upstream-llvm/clang/test/Preprocessor/Inputs/search-path-usage/FwA' > > > > 2 errors generated. > > > > ``` > > > > > > > > If I drop `@*`, stuff doesn't work (presumably because of line numbers): > > > > ``` > > > > error: 'remark' diagnostics expected but not seen: > > > > File > > > > /Users/Jan/Code/upstream-llvm/clang/test/Preprocessor/search-path-usage-headers.m > > > > Line 30: search path used: '{{.*}}/search-path-usage/FwA' > > > > error: 'remark' diagnostics seen but not expected: > > > > (frontend): search path used: > > > > '/Users/Jan/Code/upstream-llvm/clang/test/Preprocessor/Inputs/search-path-usage/FwA' > > > > 2 errors generated. > > > > ``` > > > > > > > > Let me know if I'm doing something wrong. In the meantime, I made the > > > > FileCheck checks more thorough as you suggested. > > > > - Header map (matched and used vs. matched but not used vs. not matched > > > > -- for the middle case, I'm not sure what result we actually want) > > > > > > Can you clarify what you mean by "matched" and "used" exactly? > > Oh, I hadn't realized the diagnostic doesn't have a source location. Is > > there a reason it has no location right now? > > > > Seems like pointing at the first `#include` that triggers it would be > > ideal, showing the include stack if there is one. > > > - Header map (matched and used vs. matched but not used vs. not matched > > > > Can you clarify what you mean by "matched" and "used" exactly? > > Assume "hmap" is a header map with the edge `a.h -> b.h`. > - "Matched" means the header map is queried with "a.h" (or "A.H", or > whatever). > - "Used" means "b.h" is subsequently found on disk. > > I think the way header maps work, a "matched but not used" would typically > result in a compile error... since once the path is converted to "b.h" > there's no going back. But here's a scenario would it wouldn't be an error: > ``` > lang=c++ > #if __has_include("a.h") > #include "a.h" > #endif > ``` > The `__has_include("a.h")` would match in the header map, converting to > `"b.h"`, but there's no error when `"b.h"` isn't found on disk. > > > > for the middle case, I'm not sure what result we actually want > > Thinking about it more, I think we need it to count as a use of the header > map whenever there's a match (regardless of what happens downstream after the > path replacement), because it can affect the result. > > With this setup: > ``` > % find . -type f > include/a.h > include/c.h > t.c > hmap > % clang -Ihmap -Iinclude -E t.c > ``` > (same header map, `a.h -> b.h`) > > Here's an example of the "middle" case, matched but not used, where by my > logic above the header map needs a remark: > ``` > % cat t.c > #include "a.h" > ``` > ... because removing the `-Ihmap` changes the result of the compilation. > > Here's the third case, not matched, where the remark shouldn't trigger since > `-Ihmap` does not affect the compilation: > ``` > % cat t.c > #include "c.h" > ``` > ... my point being that it should be easy to specify the location, and then you're not having to combine `@*` with `-re`: ``` #include "some-header" // \ // expected-remark-re {{source path used '{{.*}}/search-path-used/FwA'}} \ // expected-remark {{some other diagnostic on the same line}} #include "some-other-header" // expected-remark-re @-1 {{source path used '{{.*}}/search-path-used/FwA'}} ``` For the diagnostics in the headers, which might be included in other tests, probably simplest to add a marker: ``` // In the x.h #include "y.h" // #x-include-y // In the test // expected-remark-re @#x-include-y {{...} #include "x.h" // expected-note {{...}} ``` but you could also use `-verify=name-of-test` and use `name-of-test-remark` in x.h to avoid affecting other tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102923/new/ https://reviews.llvm.org/D102923 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits