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

[Christopher Di Bella via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG91be60b34715: Respect 
-fdiagnostics-absolute-paths on emit include location (authored by 
charmitro, committed by cjdb).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151833

Files:
  clang/lib/Frontend/TextDiagnostic.cpp
  clang/test/Frontend/absolute-paths-import.h
  clang/test/Frontend/absolute-paths.c


Index: clang/test/Frontend/absolute-paths.c
===
--- clang/test/Frontend/absolute-paths.c
+++ clang/test/Frontend/absolute-paths.c
@@ -1,5 +1,10 @@
-// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. %s 2>&1 | 
FileCheck -check-prefix=NORMAL -check-prefix=CHECK %s
-// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. 
-fdiagnostics-absolute-paths %s 2>&1 | FileCheck -check-prefix=ABSOLUTE 
-check-prefix=CHECK %s
+// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. %s 2>&1 | 
FileCheck -DROOT_ABSOLUTE=%s -check-prefix=NORMAL -check-prefix=CHECK %s
+// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. 
-fdiagnostics-absolute-paths %s 2>&1 | FileCheck -DROOT_ABSOLUTE=%s 
-check-prefix=ABSOLUTE -check-prefix=CHECK %s
+
+#include "absolute-paths-import.h"
+// NORMAL: In file included from {{.*}}absolute-paths.c:4:
+// NORMAL-NOT: In file included from [[ROOT_ABSOLUTE]]:4:
+// ABSOLUTE: In file included from [[ROOT_ABSOLUTE]]:4:
 
 #include "absolute-paths.h"
 
Index: clang/test/Frontend/absolute-paths-import.h
===
--- /dev/null
+++ clang/test/Frontend/absolute-paths-import.h
@@ -0,0 +1 @@
+#warning abc
Index: clang/lib/Frontend/TextDiagnostic.cpp
===
--- clang/lib/Frontend/TextDiagnostic.cpp
+++ clang/lib/Frontend/TextDiagnostic.cpp
@@ -868,10 +868,11 @@
 }
 
 void TextDiagnostic::emitIncludeLocation(FullSourceLoc Loc, PresumedLoc PLoc) {
-  if (DiagOpts->ShowLocation && PLoc.isValid())
-OS << "In file included from " << PLoc.getFilename() << ':'
-   << PLoc.getLine() << ":\n";
-  else
+  if (DiagOpts->ShowLocation && PLoc.isValid()) {
+OS << "In file included from ";
+emitFilename(PLoc.getFilename(), Loc.getManager());
+OS << ':' << PLoc.getLine() << ":\n";
+  } else
 OS << "In included file:\n";
 }
 


Index: clang/test/Frontend/absolute-paths.c
===
--- clang/test/Frontend/absolute-paths.c
+++ clang/test/Frontend/absolute-paths.c
@@ -1,5 +1,10 @@
-// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. %s 2>&1 | FileCheck -check-prefix=NORMAL -check-prefix=CHECK %s
-// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. -fdiagnostics-absolute-paths %s 2>&1 | FileCheck -check-prefix=ABSOLUTE -check-prefix=CHECK %s
+// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. %s 2>&1 | FileCheck -DROOT_ABSOLUTE=%s -check-prefix=NORMAL -check-prefix=CHECK %s
+// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. -fdiagnostics-absolute-paths %s 2>&1 | FileCheck -DROOT_ABSOLUTE=%s -check-prefix=ABSOLUTE -check-prefix=CHECK %s
+
+#include "absolute-paths-import.h"
+// NORMAL: In file included from {{.*}}absolute-paths.c:4:
+// NORMAL-NOT: In file included from [[ROOT_ABSOLUTE]]:4:
+// ABSOLUTE: In file included from [[ROOT_ABSOLUTE]]:4:
 
 #include "absolute-paths.h"
 
Index: clang/test/Frontend/absolute-paths-import.h
===
--- /dev/null
+++ clang/test/Frontend/absolute-paths-import.h
@@ -0,0 +1 @@
+#warning abc
Index: clang/lib/Frontend/TextDiagnostic.cpp
===
--- clang/lib/Frontend/TextDiagnostic.cpp
+++ clang/lib/Frontend/TextDiagnostic.cpp
@@ -868,10 +868,11 @@
 }
 
 void TextDiagnostic::emitIncludeLocation(FullSourceLoc Loc, PresumedLoc PLoc) {
-  if (DiagOpts->ShowLocation && PLoc.isValid())
-OS << "In file included from " << PLoc.getFilename() << ':'
-   << PLoc.getLine() << ":\n";
-  else
+  if (DiagOpts->ShowLocation && PLoc.isValid()) {
+OS << "In file included from ";
+emitFilename(PLoc.getFilename(), Loc.getManager());
+OS << ':' << PLoc.getLine() << ":\n";
+  } else
 OS << "In included file:\n";
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added a comment.

I already have another commit I plan to push, will do these together :)


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

https://reviews.llvm.org/D151833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charalampos Mitrodimas via Phabricator via cfe-commits]

charmitro added a comment.

In D151833#4396008 , @charmitro wrote:

> In D151833#4395956 , @aaron.ballman 
> wrote:
>
>> Do you need someone to commit on your behalf? If so, what name and email 
>> address would you like used for patch attribution?
>
>
>
> In D151833#4395957 , @tbaeder wrote:
>
>> @charmitro do you have commit rights or do you need someone to push on your 
>> behalf?
>
> I do not have commit rights, since I'm planning to contribute more often, I'd 
> like commit access at some point. I can request it ASAP if needed.

Author should be:
Charalampos Mitrodimas 


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

https://reviews.llvm.org/D151833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charalampos Mitrodimas via Phabricator via cfe-commits]

charmitro added a comment.

In D151833#4396426 , @cjdb wrote:

> In D151833#4396399 , @charmitro 
> wrote:
>
>> In D151833#4396244 , @cjdb wrote:
>>
>>> Can the commit message have a description please? It's unclear to me why 
>>> this is necessary (although I'm sure there's a good reason).
>>
>> Thanks! Done, also check https://github.com/llvm/llvm-project/issues/63026
>
> Nice, thanks! Would you mind adding `Fixes #63026.` somewhere in the commit 
> message? That will auto-close the issue upon merge.

Thanks for pointing that, done.


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

https://reviews.llvm.org/D151833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added a comment.

In D151833#4396399 , @charmitro wrote:

> In D151833#4396244 , @cjdb wrote:
>
>> Can the commit message have a description please? It's unclear to me why 
>> this is necessary (although I'm sure there's a good reason).
>
> Thanks! Done, also check https://github.com/llvm/llvm-project/issues/63026

Nice, thanks! Would you mind adding `Fixes #63026.` somewhere in the commit 
message? That will auto-close the issue upon merge.


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

https://reviews.llvm.org/D151833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charalampos Mitrodimas via Phabricator via cfe-commits]

charmitro added a comment.

In D151833#4396244 , @cjdb wrote:

> Can the commit message have a description please? It's unclear to me why this 
> is necessary (although I'm sure there's a good reason).

Thanks! Done, also check https://github.com/llvm/llvm-project/issues/63026


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

https://reviews.llvm.org/D151833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added a comment.

Can the commit message have a description please? It's unclear to me why this 
is necessary (although I'm sure there's a good reason).


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

https://reviews.llvm.org/D151833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charalampos Mitrodimas via Phabricator via cfe-commits]

charmitro marked an inline comment as done.
charmitro added a comment.

In D151833#4395956 , @aaron.ballman 
wrote:

> Do you need someone to commit on your behalf? If so, what name and email 
> address would you like used for patch attribution?



In D151833#4395957 , @tbaeder wrote:

> @charmitro do you have commit rights or do you need someone to push on your 
> behalf?

I do not have commit rights, since I'm planning to contribute more often, I'd 
like commit access at some point. I can request it ASAP if needed.


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

https://reviews.llvm.org/D151833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

tbaeder added a comment.

@charmitro do you have commit rights or do you need someone to push on your 
behalf?


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

https://reviews.llvm.org/D151833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Do you need someone to commit on your behalf? If so, what name and email 
address would you like used for patch attribution?


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

https://reviews.llvm.org/D151833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!


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

https://reviews.llvm.org/D151833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

tbaeder added subscribers: cjdb, aaron.ballman.
tbaeder added a comment.

LGTM, but maybe wait for a response from @aaron.ballman or @cjdb


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

https://reviews.llvm.org/D151833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charalampos Mitrodimas via Phabricator via cfe-commits]

charmitro updated this revision to Diff 528219.

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

https://reviews.llvm.org/D151833

Files:
  clang/lib/Frontend/TextDiagnostic.cpp
  clang/test/Frontend/absolute-paths-import.h
  clang/test/Frontend/absolute-paths.c


Index: clang/test/Frontend/absolute-paths.c
===
--- clang/test/Frontend/absolute-paths.c
+++ clang/test/Frontend/absolute-paths.c
@@ -1,5 +1,10 @@
-// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. %s 2>&1 | 
FileCheck -check-prefix=NORMAL -check-prefix=CHECK %s
-// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. 
-fdiagnostics-absolute-paths %s 2>&1 | FileCheck -check-prefix=ABSOLUTE 
-check-prefix=CHECK %s
+// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. %s 2>&1 | 
FileCheck -DROOT_ABSOLUTE=%s -check-prefix=NORMAL -check-prefix=CHECK %s
+// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. 
-fdiagnostics-absolute-paths %s 2>&1 | FileCheck -DROOT_ABSOLUTE=%s 
-check-prefix=ABSOLUTE -check-prefix=CHECK %s
+
+#include "absolute-paths-import.h"
+// NORMAL: In file included from {{.*}}absolute-paths.c:4:
+// NORMAL-NOT: In file included from [[ROOT_ABSOLUTE]]:4:
+// ABSOLUTE: In file included from [[ROOT_ABSOLUTE]]:4:
 
 #include "absolute-paths.h"
 
Index: clang/test/Frontend/absolute-paths-import.h
===
--- /dev/null
+++ clang/test/Frontend/absolute-paths-import.h
@@ -0,0 +1 @@
+#warning abc
Index: clang/lib/Frontend/TextDiagnostic.cpp
===
--- clang/lib/Frontend/TextDiagnostic.cpp
+++ clang/lib/Frontend/TextDiagnostic.cpp
@@ -868,10 +868,11 @@
 }
 
 void TextDiagnostic::emitIncludeLocation(FullSourceLoc Loc, PresumedLoc PLoc) {
-  if (DiagOpts->ShowLocation && PLoc.isValid())
-OS << "In file included from " << PLoc.getFilename() << ':'
-   << PLoc.getLine() << ":\n";
-  else
+  if (DiagOpts->ShowLocation && PLoc.isValid()) {
+OS << "In file included from ";
+emitFilename(PLoc.getFilename(), Loc.getManager());
+OS << ':' << PLoc.getLine() << ":\n";
+  } else
 OS << "In included file:\n";
 }
 


Index: clang/test/Frontend/absolute-paths.c
===
--- clang/test/Frontend/absolute-paths.c
+++ clang/test/Frontend/absolute-paths.c
@@ -1,5 +1,10 @@
-// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. %s 2>&1 | FileCheck -check-prefix=NORMAL -check-prefix=CHECK %s
-// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. -fdiagnostics-absolute-paths %s 2>&1 | FileCheck -check-prefix=ABSOLUTE -check-prefix=CHECK %s
+// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. %s 2>&1 | FileCheck -DROOT_ABSOLUTE=%s -check-prefix=NORMAL -check-prefix=CHECK %s
+// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. -fdiagnostics-absolute-paths %s 2>&1 | FileCheck -DROOT_ABSOLUTE=%s -check-prefix=ABSOLUTE -check-prefix=CHECK %s
+
+#include "absolute-paths-import.h"
+// NORMAL: In file included from {{.*}}absolute-paths.c:4:
+// NORMAL-NOT: In file included from [[ROOT_ABSOLUTE]]:4:
+// ABSOLUTE: In file included from [[ROOT_ABSOLUTE]]:4:
 
 #include "absolute-paths.h"
 
Index: clang/test/Frontend/absolute-paths-import.h
===
--- /dev/null
+++ clang/test/Frontend/absolute-paths-import.h
@@ -0,0 +1 @@
+#warning abc
Index: clang/lib/Frontend/TextDiagnostic.cpp
===
--- clang/lib/Frontend/TextDiagnostic.cpp
+++ clang/lib/Frontend/TextDiagnostic.cpp
@@ -868,10 +868,11 @@
 }
 
 void TextDiagnostic::emitIncludeLocation(FullSourceLoc Loc, PresumedLoc PLoc) {
-  if (DiagOpts->ShowLocation && PLoc.isValid())
-OS << "In file included from " << PLoc.getFilename() << ':'
-   << PLoc.getLine() << ":\n";
-  else
+  if (DiagOpts->ShowLocation && PLoc.isValid()) {
+OS << "In file included from ";
+emitFilename(PLoc.getFilename(), Loc.getManager());
+OS << ':' << PLoc.getLine() << ":\n";
+  } else
 OS << "In included file:\n";
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charalampos Mitrodimas via Phabricator via cfe-commits]

charmitro 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:
 

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.


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

https://reviews.llvm.org/D151833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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:
> > > > 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.


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

https://reviews.llvm.org/D151833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charalampos Mitrodimas via Phabricator via cfe-commits]

charmitro updated this revision to Diff 527914.

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

https://reviews.llvm.org/D151833

Files:
  clang/lib/Frontend/TextDiagnostic.cpp
  clang/test/Frontend/absolute-paths-import.h
  clang/test/Frontend/absolute-paths.c


Index: clang/test/Frontend/absolute-paths.c
===
--- clang/test/Frontend/absolute-paths.c
+++ clang/test/Frontend/absolute-paths.c
@@ -1,5 +1,9 @@
 // RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. %s 2>&1 | 
FileCheck -check-prefix=NORMAL -check-prefix=CHECK %s
-// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. 
-fdiagnostics-absolute-paths %s 2>&1 | FileCheck -check-prefix=ABSOLUTE 
-check-prefix=CHECK %s
+// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. 
-fdiagnostics-absolute-paths %s 2>&1 | FileCheck -DROOT_ABSOLUTE=%s 
-check-prefix=ABSOLUTE -check-prefix=CHECK %s
+
+#include "absolute-paths-import.h"
+// NORMAL: In file included from {{.*absolute-paths.c:4}}:
+// ABSOLUTE: In file included from [[ROOT_ABSOLUTE]]:4:
 
 #include "absolute-paths.h"
 
Index: clang/test/Frontend/absolute-paths-import.h
===
--- /dev/null
+++ clang/test/Frontend/absolute-paths-import.h
@@ -0,0 +1 @@
+#warning abc
Index: clang/lib/Frontend/TextDiagnostic.cpp
===
--- clang/lib/Frontend/TextDiagnostic.cpp
+++ clang/lib/Frontend/TextDiagnostic.cpp
@@ -868,10 +868,11 @@
 }
 
 void TextDiagnostic::emitIncludeLocation(FullSourceLoc Loc, PresumedLoc PLoc) {
-  if (DiagOpts->ShowLocation && PLoc.isValid())
-OS << "In file included from " << PLoc.getFilename() << ':'
-   << PLoc.getLine() << ":\n";
-  else
+  if (DiagOpts->ShowLocation && PLoc.isValid()) {
+OS << "In file included from ";
+emitFilename(PLoc.getFilename(), Loc.getManager());
+OS << ':' << PLoc.getLine() << ":\n";
+  } else
 OS << "In included file:\n";
 }
 


Index: clang/test/Frontend/absolute-paths.c
===
--- clang/test/Frontend/absolute-paths.c
+++ clang/test/Frontend/absolute-paths.c
@@ -1,5 +1,9 @@
 // RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. %s 2>&1 | FileCheck -check-prefix=NORMAL -check-prefix=CHECK %s
-// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. -fdiagnostics-absolute-paths %s 2>&1 | FileCheck -check-prefix=ABSOLUTE -check-prefix=CHECK %s
+// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. -fdiagnostics-absolute-paths %s 2>&1 | FileCheck -DROOT_ABSOLUTE=%s -check-prefix=ABSOLUTE -check-prefix=CHECK %s
+
+#include "absolute-paths-import.h"
+// NORMAL: In file included from {{.*absolute-paths.c:4}}:
+// ABSOLUTE: In file included from [[ROOT_ABSOLUTE]]:4:
 
 #include "absolute-paths.h"
 
Index: clang/test/Frontend/absolute-paths-import.h
===
--- /dev/null
+++ clang/test/Frontend/absolute-paths-import.h
@@ -0,0 +1 @@
+#warning abc
Index: clang/lib/Frontend/TextDiagnostic.cpp
===
--- clang/lib/Frontend/TextDiagnostic.cpp
+++ clang/lib/Frontend/TextDiagnostic.cpp
@@ -868,10 +868,11 @@
 }
 
 void TextDiagnostic::emitIncludeLocation(FullSourceLoc Loc, PresumedLoc PLoc) {
-  if (DiagOpts->ShowLocation && PLoc.isValid())
-OS << "In file included from " << PLoc.getFilename() << ':'
-   << PLoc.getLine() << ":\n";
-  else
+  if (DiagOpts->ShowLocation && PLoc.isValid()) {
+OS << "In file included from ";
+emitFilename(PLoc.getFilename(), Loc.getManager());
+OS << ':' << PLoc.getLine() << ":\n";
+  } else
 OS << "In included file:\n";
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charalampos Mitrodimas via Phabricator via cfe-commits]

charmitro 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:
 

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`?


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

https://reviews.llvm.org/D151833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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:
> > 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]]`?


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

https://reviews.llvm.org/D151833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charalampos Mitrodimas via Phabricator via cfe-commits]

charmitro 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:
 

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 {{.*}}:
```


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

https://reviews.llvm.org/D151833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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:
 

This checks the same thing in both cases, but in the `NORMAL` case, it should 
//not// use the absolute path, shouldn't it?


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

https://reviews.llvm.org/D151833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charalampos Mitrodimas via Phabricator via cfe-commits]

charmitro updated this revision to Diff 527828.

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

https://reviews.llvm.org/D151833

Files:
  clang/lib/Frontend/TextDiagnostic.cpp
  clang/test/Frontend/absolute-paths-import.h
  clang/test/Frontend/absolute-paths.c


Index: clang/test/Frontend/absolute-paths.c
===
--- clang/test/Frontend/absolute-paths.c
+++ clang/test/Frontend/absolute-paths.c
@@ -1,5 +1,9 @@
-// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. %s 2>&1 | 
FileCheck -check-prefix=NORMAL -check-prefix=CHECK %s
-// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. 
-fdiagnostics-absolute-paths %s 2>&1 | FileCheck -check-prefix=ABSOLUTE 
-check-prefix=CHECK %s
+// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. %s 2>&1 | 
FileCheck -DROOT_ABSOLUTE=%s -check-prefix=NORMAL -check-prefix=CHECK %s
+// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. 
-fdiagnostics-absolute-paths %s 2>&1 | FileCheck -DROOT_ABSOLUTE=%s 
-check-prefix=ABSOLUTE -check-prefix=CHECK %s
+
+#include "absolute-paths-import.h"
+// NORMAL: In file included from [[ROOT_ABSOLUTE]]:4:
+// ABSOLUTE: In file included from [[ROOT_ABSOLUTE]]:4:
 
 #include "absolute-paths.h"
 
Index: clang/test/Frontend/absolute-paths-import.h
===
--- /dev/null
+++ clang/test/Frontend/absolute-paths-import.h
@@ -0,0 +1 @@
+#warning abc
Index: clang/lib/Frontend/TextDiagnostic.cpp
===
--- clang/lib/Frontend/TextDiagnostic.cpp
+++ clang/lib/Frontend/TextDiagnostic.cpp
@@ -868,10 +868,11 @@
 }
 
 void TextDiagnostic::emitIncludeLocation(FullSourceLoc Loc, PresumedLoc PLoc) {
-  if (DiagOpts->ShowLocation && PLoc.isValid())
-OS << "In file included from " << PLoc.getFilename() << ':'
-   << PLoc.getLine() << ":\n";
-  else
+  if (DiagOpts->ShowLocation && PLoc.isValid()) {
+OS << "In file included from ";
+emitFilename(PLoc.getFilename(), Loc.getManager());
+OS << ':' << PLoc.getLine() << ":\n";
+  } else
 OS << "In included file:\n";
 }
 


Index: clang/test/Frontend/absolute-paths.c
===
--- clang/test/Frontend/absolute-paths.c
+++ clang/test/Frontend/absolute-paths.c
@@ -1,5 +1,9 @@
-// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. %s 2>&1 | FileCheck -check-prefix=NORMAL -check-prefix=CHECK %s
-// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. -fdiagnostics-absolute-paths %s 2>&1 | FileCheck -check-prefix=ABSOLUTE -check-prefix=CHECK %s
+// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. %s 2>&1 | FileCheck -DROOT_ABSOLUTE=%s -check-prefix=NORMAL -check-prefix=CHECK %s
+// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. -fdiagnostics-absolute-paths %s 2>&1 | FileCheck -DROOT_ABSOLUTE=%s -check-prefix=ABSOLUTE -check-prefix=CHECK %s
+
+#include "absolute-paths-import.h"
+// NORMAL: In file included from [[ROOT_ABSOLUTE]]:4:
+// ABSOLUTE: In file included from [[ROOT_ABSOLUTE]]:4:
 
 #include "absolute-paths.h"
 
Index: clang/test/Frontend/absolute-paths-import.h
===
--- /dev/null
+++ clang/test/Frontend/absolute-paths-import.h
@@ -0,0 +1 @@
+#warning abc
Index: clang/lib/Frontend/TextDiagnostic.cpp
===
--- clang/lib/Frontend/TextDiagnostic.cpp
+++ clang/lib/Frontend/TextDiagnostic.cpp
@@ -868,10 +868,11 @@
 }
 
 void TextDiagnostic::emitIncludeLocation(FullSourceLoc Loc, PresumedLoc PLoc) {
-  if (DiagOpts->ShowLocation && PLoc.isValid())
-OS << "In file included from " << PLoc.getFilename() << ':'
-   << PLoc.getLine() << ":\n";
-  else
+  if (DiagOpts->ShowLocation && PLoc.isValid()) {
+OS << "In file included from ";
+emitFilename(PLoc.getFilename(), Loc.getManager());
+OS << ':' << PLoc.getLine() << ":\n";
+  } else
 OS << "In included file:\n";
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charalampos Mitrodimas via Phabricator via cfe-commits]

charmitro added a comment.

In D151833#4390279 , @charmitro wrote:

> In D151833#4386128 , @tbaeder wrote:
>
>> Can you add a test case for the change? Looks like there's something similar 
>> already in `clang/test/Frontend/absolute-paths.c`
>
> Is it possible to use substitutions inside let's say `NORMAL: `? How could I 
> resolve the absolute path without hard-coding my path inside the test case?

I was able to make it work using `-DNAME=` in the FileCheck invocation 
inside `absolute-paths.c` test file.


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

https://reviews.llvm.org/D151833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charalampos Mitrodimas via Phabricator via cfe-commits]

charmitro updated this revision to Diff 527821.

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

https://reviews.llvm.org/D151833

Files:
  clang/lib/Frontend/TextDiagnostic.cpp
  clang/test/Frontend/absolute-paths-import.h
  clang/test/Frontend/absolute-paths.c


Index: clang/test/Frontend/absolute-paths.c
===
--- clang/test/Frontend/absolute-paths.c
+++ clang/test/Frontend/absolute-paths.c
@@ -1,5 +1,9 @@
-// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. %s 2>&1 | 
FileCheck -check-prefix=NORMAL -check-prefix=CHECK %s
-// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. 
-fdiagnostics-absolute-paths %s 2>&1 | FileCheck -check-prefix=ABSOLUTE 
-check-prefix=CHECK %s
+// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. %s 2>&1 | 
FileCheck -DROOT_ABSOLUTE=%s -check-prefix=NORMAL -check-prefix=CHECK %s
+// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. 
-fdiagnostics-absolute-paths %s 2>&1 | FileCheck -DROOT_ABSOLUTE=%s 
-check-prefix=ABSOLUTE -check-prefix=CHECK %s --dump-input-filter=all
+
+#include "absolute-paths-import.h"
+// NORMAL: In file included from [[ROOT_ABSOLUTE]]:4:
+// ABSOLUTE: In file included from [[ROOT_ABSOLUTE]]:4:
 
 #include "absolute-paths.h"
 
Index: clang/test/Frontend/absolute-paths-import.h
===
--- /dev/null
+++ clang/test/Frontend/absolute-paths-import.h
@@ -0,0 +1 @@
+#warning abc
Index: clang/lib/Frontend/TextDiagnostic.cpp
===
--- clang/lib/Frontend/TextDiagnostic.cpp
+++ clang/lib/Frontend/TextDiagnostic.cpp
@@ -868,10 +868,11 @@
 }
 
 void TextDiagnostic::emitIncludeLocation(FullSourceLoc Loc, PresumedLoc PLoc) {
-  if (DiagOpts->ShowLocation && PLoc.isValid())
-OS << "In file included from " << PLoc.getFilename() << ':'
-   << PLoc.getLine() << ":\n";
-  else
+  if (DiagOpts->ShowLocation && PLoc.isValid()) {
+OS << "In file included from ";
+emitFilename(PLoc.getFilename(), Loc.getManager());
+OS << ':' << PLoc.getLine() << ":\n";
+  } else
 OS << "In included file:\n";
 }
 


Index: clang/test/Frontend/absolute-paths.c
===
--- clang/test/Frontend/absolute-paths.c
+++ clang/test/Frontend/absolute-paths.c
@@ -1,5 +1,9 @@
-// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. %s 2>&1 | FileCheck -check-prefix=NORMAL -check-prefix=CHECK %s
-// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. -fdiagnostics-absolute-paths %s 2>&1 | FileCheck -check-prefix=ABSOLUTE -check-prefix=CHECK %s
+// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. %s 2>&1 | FileCheck -DROOT_ABSOLUTE=%s -check-prefix=NORMAL -check-prefix=CHECK %s
+// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SystemHeaderPrefix/.. -fdiagnostics-absolute-paths %s 2>&1 | FileCheck -DROOT_ABSOLUTE=%s -check-prefix=ABSOLUTE -check-prefix=CHECK %s --dump-input-filter=all
+
+#include "absolute-paths-import.h"
+// NORMAL: In file included from [[ROOT_ABSOLUTE]]:4:
+// ABSOLUTE: In file included from [[ROOT_ABSOLUTE]]:4:
 
 #include "absolute-paths.h"
 
Index: clang/test/Frontend/absolute-paths-import.h
===
--- /dev/null
+++ clang/test/Frontend/absolute-paths-import.h
@@ -0,0 +1 @@
+#warning abc
Index: clang/lib/Frontend/TextDiagnostic.cpp
===
--- clang/lib/Frontend/TextDiagnostic.cpp
+++ clang/lib/Frontend/TextDiagnostic.cpp
@@ -868,10 +868,11 @@
 }
 
 void TextDiagnostic::emitIncludeLocation(FullSourceLoc Loc, PresumedLoc PLoc) {
-  if (DiagOpts->ShowLocation && PLoc.isValid())
-OS << "In file included from " << PLoc.getFilename() << ':'
-   << PLoc.getLine() << ":\n";
-  else
+  if (DiagOpts->ShowLocation && PLoc.isValid()) {
+OS << "In file included from ";
+emitFilename(PLoc.getFilename(), Loc.getManager());
+OS << ':' << PLoc.getLine() << ":\n";
+  } else
 OS << "In included file:\n";
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charalampos Mitrodimas via Phabricator via cfe-commits]

charmitro added a comment.

In D151833#4386128 , @tbaeder wrote:

> Can you add a test case for the change? Looks like there's something similar 
> already in `clang/test/Frontend/absolute-paths.c`

Is it possible to use substitutions inside let's say `NORMAL: `? How could I 
resolve the absolute path without hard-coding my path inside the test case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

tbaeder added a comment.

Can you add a test case for the change? Looks like there's something similar 
already in `clang/test/Frontend/absolute-paths.c`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charalampos Mitrodimas via Phabricator via cfe-commits]

charmitro created this revision.
Herald added a subscriber: kristof.beyls.
Herald added a project: All.
charmitro requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Signed-off-by: Charalampos Mitrodimas 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151833

Files:
  clang/lib/Frontend/TextDiagnostic.cpp


Index: clang/lib/Frontend/TextDiagnostic.cpp
===
--- clang/lib/Frontend/TextDiagnostic.cpp
+++ clang/lib/Frontend/TextDiagnostic.cpp
@@ -896,10 +896,11 @@
 }
 
 void TextDiagnostic::emitIncludeLocation(FullSourceLoc Loc, PresumedLoc PLoc) {
-  if (DiagOpts->ShowLocation && PLoc.isValid())
-OS << "In file included from " << PLoc.getFilename() << ':'
-   << PLoc.getLine() << ":\n";
-  else
+  if (DiagOpts->ShowLocation && PLoc.isValid()) {
+OS << "In file included from ";
+emitFilename(PLoc.getFilename(), Loc.getManager());
+OS << ':' << PLoc.getLine() << ":\n";
+  } else
 OS << "In included file:\n";
 }
 


Index: clang/lib/Frontend/TextDiagnostic.cpp
===
--- clang/lib/Frontend/TextDiagnostic.cpp
+++ clang/lib/Frontend/TextDiagnostic.cpp
@@ -896,10 +896,11 @@
 }
 
 void TextDiagnostic::emitIncludeLocation(FullSourceLoc Loc, PresumedLoc PLoc) {
-  if (DiagOpts->ShowLocation && PLoc.isValid())
-OS << "In file included from " << PLoc.getFilename() << ':'
-   << PLoc.getLine() << ":\n";
-  else
+  if (DiagOpts->ShowLocation && PLoc.isValid()) {
+OS << "In file included from ";
+emitFilename(PLoc.getFilename(), Loc.getManager());
+OS << ':' << PLoc.getLine() << ":\n";
+  } else
 OS << "In included file:\n";
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits