[PATCH] D151833: Respect "-fdiagnostics-absolute-paths" on emit include location

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.


================
Comment at: clang/test/Frontend/absolute-paths.c:6
+// NORMAL: In file included from [[ROOT_ABSOLUTE]]:4:
+// ABSOLUTE: In file included from [[ROOT_ABSOLUTE]]:4:
 
----------------
charmitro wrote:
> tbaeder wrote:
> > charmitro wrote:
> > > tbaeder wrote:
> > > > charmitro wrote:
> > > > > tbaeder wrote:
> > > > > > This checks the same thing in both cases, but in the `NORMAL` case, it should //not// use the absolute path, shouldn't it?
> > > > > You're correct. I was constantly testing it from the same path.
> > > > > 
> > > > > Since the relative path is going to be different depending on where you running from, would it be wise to accept a regular expression of any string in the `NORMAL` case?
> > > > > 
> > > > > For example, 
> > > > > ```
> > > > > NORMAL: In file included from {{.*}}:
> > > > > ```
> > > > I wonder if it would make sense to just check for the filename in the `NORMAL` case and then do a `NORMAL-NOT: [[ROOT_ABSOLUTE]]`?
> > > Yes, that way we are testing if the warning contains the correct filename+LOC.
> > > 
> > > Something like: `// NORMAL: In file included from {{.*absolute-paths.c:4}}:`.
> > > 
> > > But why changefrom `ABSOLUTE:` to `NORMAL-NOT`?
> > Your regex checks if the path ends with the file name, right? That would be true in both cases.
> What else do you propose here?
> 
> Let me explain why this was the only obvious option for me.
> 
> In the case of `NORMAL`, it checks for the relative path based on the location you invoke the test commands. 
> 
> Given that, if we, for example,
> 1. run `llvm-lit` from the project root, the `NORMAL` case expects:
> ```
> In file included from clang/test/Frontend/absolute-paths.c:4:
> ```
> 2. run `llvm-lit` from `$PROJECT_ROOT/clang/test/`, the `NORMAL` case expects:
> ```
> In file included from Frontend/absolute-paths.c:4:
> ```
> 
> It seems unwise to me to hard-code the path based on what the CI excepts in the `NORMAL` scenario, because that way, whoever runs the test cases manually, will experience test failure.
You can still keep the `ABSOLUTE: [[ROOT_ABSOLUTE]]` stuff, I was just proposing to add the `NORMAL-NOT` one additionally, so we can make sure we're not printing the absolute path in the normal case as well.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151833/new/

https://reviews.llvm.org/D151833