[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Looks like this change is coming up in: 
https://github.com/llvm/llvm-project/issues/63284


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Ken Matsui via Phabricator via cfe-commits]

ken-matsui added a comment.

Hello @Jake-Egan,

I believe the problem was already fixed by the @aaron.ballman's patch here 
.

Thank you for fixing it, @aaron.ballman!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Jake Egan via Phabricator via cfe-commits]

Jake-Egan added a comment.

Hi, this patch causes a test failure on AIX 
https://lab.llvm.org/buildbot/#/builders/214/builds/1221/steps/6/logs/FAIL__Clang__noinline_cu

Could you please make a fix or revert if it takes too long?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Ken Matsui via Phabricator via cfe-commits]

ken-matsui added a comment.



> That looks to be exactly what I needed, but you can double-check that you 
> were properly attributed here: 
> https://github.com/llvm/llvm-project/commit/786c721c2bbd2e0646e314671e010859550423bf
> I added one when I landed. I also adjusted the commit message slightly from 
> what you have here in Phab for some added clarity.

Thank you! I was able to confirm that I was attributed.

In D124534#3505729 , @aaron.ballman 
wrote:

> In D124534#3505709 , @aaron.ballman 
> wrote:
>
>> In D124534#3504641 , @ken-matsui 
>> wrote:
>>
>>> (I use this info for every commit on GitHub, but is this what you expected? 
>>> Please let me know if you need an email where you can be reached out.)
>>
>> That looks to be exactly what I needed, but you can double-check that you 
>> were properly attributed here: 
>> https://github.com/llvm/llvm-project/commit/786c721c2bbd2e0646e314671e010859550423bf
>
> Actually, it does raise an interesting issue -- I don't think you'll get 
> build failure emails when using that address. After a patch is landed, a 
> bunch of build bots test the patch out and when there's a failure, they send 
> email to everyone on the blame list. You should probably use an email address 
> that receives email for future commits (or when you go to obtain your own 
> commit privileges) so that you can receive those emails.

Ah, sorry. I also have a public email for GitHub (just mostly used the private 
one), so I'll share a public one in the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D124534#3505709 , @aaron.ballman 
wrote:

> In D124534#3504641 , @ken-matsui 
> wrote:
>
>> (I use this info for every commit on GitHub, but is this what you expected? 
>> Please let me know if you need an email where you can be reached out.)
>
> That looks to be exactly what I needed, but you can double-check that you 
> were properly attributed here: 
> https://github.com/llvm/llvm-project/commit/786c721c2bbd2e0646e314671e010859550423bf

Actually, it does raise an interesting issue -- I don't think you'll get build 
failure emails when using that address. After a patch is landed, a bunch of 
build bots test the patch out and when there's a failure, they send email to 
everyone on the blame list. You should probably use an email address that 
receives email for future commits (or when you go to obtain your own commit 
privileges) so that you can receive those emails.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D124534#3504641 , @ken-matsui 
wrote:

> In D124534#3504610 , @aaron.ballman 
> wrote:
>
>> LGTM, thank you for all the hard work on this! I assume you need me to land 
>> this on your behalf -- if so, can you let me know what name and email 
>> address you would like me to use for patch attribution?
>
> Thank you so much for your support and approval!

Happy to help!

> Yes, I do not have commit access; could you please use the following info for 
> this patch?
>
> Name: Ken Matsui
> Email: 26405363+ken-mat...@users.noreply.github.com
>
> (I use this info for every commit on GitHub, but is this what you expected? 
> Please let me know if you need an email where you can be reached out.)

That looks to be exactly what I needed, but you can double-check that you were 
properly attributed here: 
https://github.com/llvm/llvm-project/commit/786c721c2bbd2e0646e314671e010859550423bf

>> I think we should add a release note for this to let folks know about the 
>> new warning group (there's a "Improvements to Clang's diagnostics" in 
>> clang/docs/ReleaseNotes.rst). You can either add one to this patch, or I can 
>> add one for you when I go to land, I'm fine either way.
>
> Could you please add it to this patch for this time?
> I would like to add one next time by referring to how you do.

I added one when I landed. I also adjusted the commit message slightly from 
what you have here in Phab for some added clarity.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Aaron Ballman via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG786c721c2bbd: Add extension diagnostic for linemarker 
directives (authored by ken-matsui, committed by aaron.ballman).

Changed prior to commit:
  https://reviews.llvm.org/D124534?vs=428450=428615#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticLexKinds.td
  clang/lib/Lex/PPDirectives.cpp
  clang/test/Preprocessor/line-directive-system-headers.c
  clang/test/Preprocessor/line-directive.c
  clang/test/SemaCXX/constexpr-string.cpp

Index: clang/test/SemaCXX/constexpr-string.cpp
===
--- clang/test/SemaCXX/constexpr-string.cpp
+++ clang/test/SemaCXX/constexpr-string.cpp
@@ -7,7 +7,7 @@
 // RUN: %clang_cc1 %s -triple armebv7-unknown-linux -std=c++2a -fsyntax-only -verify -pedantic -Wno-vla-extension -fno-signed-char
 // RUN: %clang_cc1 %s -triple armebv7-unknown-linux -std=c++2a -fsyntax-only -verify -pedantic -Wno-vla-extension -fno-wchar -DNO_PREDEFINED_WCHAR_T
 
-# 9 "/usr/include/string.h" 1 3 4
+# 9 "/usr/include/string.h" 1 3 4  // expected-warning {{this style of line directive is a GNU extension}}
 extern "C" {
   typedef decltype(sizeof(int)) size_t;
 
@@ -29,7 +29,7 @@
 }
 # 25 "SemaCXX/constexpr-string.cpp" 2
 
-# 27 "/usr/include/wchar.h" 1 3 4
+# 27 "/usr/include/wchar.h" 1 3 4  // expected-warning {{this style of line directive is a GNU extension}}
 extern "C" {
 #if NO_PREDEFINED_WCHAR_T
   typedef decltype(L'0') wchar_t;
Index: clang/test/Preprocessor/line-directive.c
===
--- clang/test/Preprocessor/line-directive.c
+++ clang/test/Preprocessor/line-directive.c
@@ -9,8 +9,8 @@
 # 20 "" 2
 
 // a push/pop before any other line control
-# 10 "enter-0" 1
-# 11 "" 2 // pop to main file
+# 10 "enter-0" 1 // expected-warning {{this style of line directive is a GNU extension}}
+# 11 "" 2 // pop to main file: expected-warning {{this style of line directive is a GNU extension}}
 #error MAIN7
 // expected-error@-1{{MAIN7}}
 
@@ -27,13 +27,15 @@
 #define A 42 "foo"
 #line A
 
-# 42
-# 42 "foo"
+# 42 // expected-warning {{this style of line directive is a GNU extension}}
+# 42 "foo" // expected-warning {{this style of line directive is a GNU extension}}
 # 42 "foo" 2 // expected-error {{invalid line marker flag '2': cannot pop empty include stack}}
+// The next two lines do not get diagnosed because they are considered to be
+// within the system header, where diagnostics are suppressed.
 # 42 "foo" 1 3  // enter
 # 42 "foo" 2 3  // exit
 # 42 "foo" 2 3 4 // expected-error {{invalid line marker flag '2': cannot pop empty include stack}}
-# 42 "foo" 3 4
+# 42 "foo" 3 4 // expected-warning {{this style of line directive is a GNU extension}}
 
 # 'a'// expected-error {{invalid preprocessing directive}}
 # 42 'f' // expected-error {{invalid filename for line marker directive}}
@@ -54,7 +56,7 @@
 
 // Verify that linemarker diddling of the system header flag works.
 
-# 192 "glomp.h" // not a system header.
+# 192 "glomp.h" // not a system header.: expected-warning {{this style of line directive is a GNU extension}}
 typedef int x;  // expected-note {{previous definition is here}}
 typedef int x;  // expected-warning {{redefinition of typedef 'x' is a C11 feature}}
 
@@ -97,7 +99,7 @@
 #line 010  // expected-warning {{#line directive interprets number as decimal, not octal}}
 extern int array[__LINE__ == 10 ? 1:-1];
 
-# 020  // expected-warning {{GNU line marker directive interprets number as decimal, not octal}}
+# 020  // expected-warning {{GNU line marker directive interprets number as decimal, not octal}} expected-warning {{this style of line directive is a GNU extension}}
 extern int array_gnuline[__LINE__ == 20 ? 1:-1];
 
 /* PR3917 */
@@ -106,7 +108,7 @@
 _\
 _LINE__ == 42 ? 1: -1];  /* line marker is location of first _ */
 
-# 51
+# 51 // expected-warning {{this style of line directive is a GNU extension}}
 extern char array2_gnuline[\
 _\
 _LINE__ == 52 ? 1: -1];  /* line marker is location of first _ */
@@ -115,12 +117,12 @@
 #line 0 "line-directive.c" // expected-warning {{#line directive with zero argument is a GNU extension}}
 undefined t; // expected-error {{unknown type name 'undefined'}}
 
-# 115 "main"
-# 116 "enter-1" 1
-# 117 "enter-2" 1
-# 118 "" 2 // pop to enter-1
+# 115 "main" // expected-warning {{this style of line directive is a GNU extension}}
+# 116 "enter-1" 1 // expected-warning {{this style of line directive is a GNU extension}}
+# 117 "enter-2" 1 // expected-warning {{this style of line directive is a GNU extension}}
+# 118 "" 2 // pop to enter-1: 

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Ken Matsui via Phabricator via cfe-commits]

ken-matsui added a comment.

In D124534#3504610 , @aaron.ballman 
wrote:

> LGTM, thank you for all the hard work on this! I assume you need me to land 
> this on your behalf -- if so, can you let me know what name and email address 
> you would like me to use for patch attribution?

Thank you so much for your support and approval!

Yes, I do not have commit access; could you please use the following info for 
this patch?

Name: Ken Matsui
Email: 26405363+ken-mat...@users.noreply.github.com

(I use this info for every commit on GitHub, but is this what you expected? 
Please let me know if you need an email where you can be reached out.)

> I think we should add a release note for this to let folks know about the new 
> warning group (there's a "Improvements to Clang's diagnostics" in 
> clang/docs/ReleaseNotes.rst). You can either add one to this patch, or I can 
> add one for you when I go to land, I'm fine either way.

Could you please add it to this patch for this time?
I would like to add one next time by referring to how you do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[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, thank you for all the hard work on this! I assume you need me to land 
this on your behalf -- if so, can you let me know what name and email address 
you would like me to use for patch attribution?

I think we should add a release note for this to let folks know about the new 
warning group (there's a "Improvements to Clang's diagnostics" in 
clang/docs/ReleaseNotes.rst). You can either add one to this patch, or I can 
add one for you when I go to land, I'm fine either way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Ken Matsui via Phabricator via cfe-commits]

ken-matsui updated this revision to Diff 428450.
ken-matsui added a comment.

Fix the test as reviewed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticLexKinds.td
  clang/lib/Lex/PPDirectives.cpp
  clang/test/Preprocessor/line-directive-system-headers.c
  clang/test/Preprocessor/line-directive.c
  clang/test/SemaCXX/constexpr-string.cpp

Index: clang/test/SemaCXX/constexpr-string.cpp
===
--- clang/test/SemaCXX/constexpr-string.cpp
+++ clang/test/SemaCXX/constexpr-string.cpp
@@ -7,7 +7,7 @@
 // RUN: %clang_cc1 %s -triple armebv7-unknown-linux -std=c++2a -fsyntax-only -verify -pedantic -Wno-vla-extension -fno-signed-char
 // RUN: %clang_cc1 %s -triple armebv7-unknown-linux -std=c++2a -fsyntax-only -verify -pedantic -Wno-vla-extension -fno-wchar -DNO_PREDEFINED_WCHAR_T
 
-# 9 "/usr/include/string.h" 1 3 4
+# 9 "/usr/include/string.h" 1 3 4  // expected-warning {{this style of line directive is a GNU extension}}
 extern "C" {
   typedef decltype(sizeof(int)) size_t;
 
@@ -29,7 +29,7 @@
 }
 # 25 "SemaCXX/constexpr-string.cpp" 2
 
-# 27 "/usr/include/wchar.h" 1 3 4
+# 27 "/usr/include/wchar.h" 1 3 4  // expected-warning {{this style of line directive is a GNU extension}}
 extern "C" {
 #if NO_PREDEFINED_WCHAR_T
   typedef decltype(L'0') wchar_t;
Index: clang/test/Preprocessor/line-directive.c
===
--- clang/test/Preprocessor/line-directive.c
+++ clang/test/Preprocessor/line-directive.c
@@ -9,8 +9,8 @@
 # 20 "" 2
 
 // a push/pop before any other line control
-# 10 "enter-0" 1
-# 11 "" 2 // pop to main file
+# 10 "enter-0" 1 // expected-warning {{this style of line directive is a GNU extension}}
+# 11 "" 2 // pop to main file: expected-warning {{this style of line directive is a GNU extension}}
 #error MAIN7
 // expected-error@-1{{MAIN7}}
 
@@ -27,13 +27,15 @@
 #define A 42 "foo"
 #line A
 
-# 42
-# 42 "foo"
+# 42 // expected-warning {{this style of line directive is a GNU extension}}
+# 42 "foo" // expected-warning {{this style of line directive is a GNU extension}}
 # 42 "foo" 2 // expected-error {{invalid line marker flag '2': cannot pop empty include stack}}
+// The next two lines do not get diagnosed because they are considered to be
+// within the system header, where diagnostics are suppressed.
 # 42 "foo" 1 3  // enter
 # 42 "foo" 2 3  // exit
 # 42 "foo" 2 3 4 // expected-error {{invalid line marker flag '2': cannot pop empty include stack}}
-# 42 "foo" 3 4
+# 42 "foo" 3 4 // expected-warning {{this style of line directive is a GNU extension}}
 
 # 'a'// expected-error {{invalid preprocessing directive}}
 # 42 'f' // expected-error {{invalid filename for line marker directive}}
@@ -54,7 +56,7 @@
 
 // Verify that linemarker diddling of the system header flag works.
 
-# 192 "glomp.h" // not a system header.
+# 192 "glomp.h" // not a system header.: expected-warning {{this style of line directive is a GNU extension}}
 typedef int x;  // expected-note {{previous definition is here}}
 typedef int x;  // expected-warning {{redefinition of typedef 'x' is a C11 feature}}
 
@@ -97,7 +99,7 @@
 #line 010  // expected-warning {{#line directive interprets number as decimal, not octal}}
 extern int array[__LINE__ == 10 ? 1:-1];
 
-# 020  // expected-warning {{GNU line marker directive interprets number as decimal, not octal}}
+# 020  // expected-warning {{GNU line marker directive interprets number as decimal, not octal}} expected-warning {{this style of line directive is a GNU extension}}
 extern int array_gnuline[__LINE__ == 20 ? 1:-1];
 
 /* PR3917 */
@@ -106,7 +108,7 @@
 _\
 _LINE__ == 42 ? 1: -1];  /* line marker is location of first _ */
 
-# 51
+# 51 // expected-warning {{this style of line directive is a GNU extension}}
 extern char array2_gnuline[\
 _\
 _LINE__ == 52 ? 1: -1];  /* line marker is location of first _ */
@@ -115,12 +117,12 @@
 #line 0 "line-directive.c" // expected-warning {{#line directive with zero argument is a GNU extension}}
 undefined t; // expected-error {{unknown type name 'undefined'}}
 
-# 115 "main"
-# 116 "enter-1" 1
-# 117 "enter-2" 1
-# 118 "" 2 // pop to enter-1
+# 115 "main" // expected-warning {{this style of line directive is a GNU extension}}
+# 116 "enter-1" 1 // expected-warning {{this style of line directive is a GNU extension}}
+# 117 "enter-2" 1 // expected-warning {{this style of line directive is a GNU extension}}
+# 118 "" 2 // pop to enter-1: expected-warning {{this style of line directive is a GNU extension}}
 #error ENTER1
 // expected-error@-1{{ENTER1}}
-# 121 "" 2 // pop to "main"
+# 121 "" 2 // pop to "main": expected-warning {{this style of line directive is a GNU extension}}
 #error MAIN2
 // expected-error@-1{{MAIN2}}

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Ken Matsui via Phabricator via cfe-commits]

ken-matsui added a comment.

Thank you for your review! I'm going to fix them :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/test/Preprocessor/line-directive-system-headers.c:1-2
+// RUN: %clang_cc1 -std=c99 -fsyntax-only -pedantic -verify %s
+// RUN: %clang_cc1 -std=c99 -fsyntax-only -pedantic -verify=system 
-Wsystem-headers %s
+

There's nothing specific to C99 that I can see in the test, so I think these 
can be dropped.



Comment at: clang/test/Preprocessor/line-directive-system-headers.c:19-21
+# 42 "foo" 2 3 4
+// expected-error@-1 {{invalid line marker flag '2': cannot pop empty include 
stack}}
+// system-error@-2 {{invalid line marker flag '2': cannot pop empty include 
stack}}

Might as well make this consistent with the other parts of the test and use 
`@#6` instead of `@-1`. Same applies elsewhere.



Comment at: clang/test/Preprocessor/line-directive-system-headers.c:40-50
+// no expected warning.
+// system-warning@#8 {{this style of line directive is a GNU extension}}
+typedef int y;
+// no expected warning.
+// system-note@-2 {{previous definition is here}}
+typedef int y;
+// no expected warning.

Let's change "no expected warning" into "Warnings silenced when 
-Wsystem-headers isn't passed." so it sounds less like "we don't expect the 
warning directly below this comment to happen".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Ken Matsui via Phabricator via cfe-commits]

ken-matsui updated this revision to Diff 428382.
ken-matsui added a comment.

Update test codes as reviewed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticLexKinds.td
  clang/lib/Lex/PPDirectives.cpp
  clang/test/Preprocessor/line-directive-system-headers.c
  clang/test/Preprocessor/line-directive.c
  clang/test/SemaCXX/constexpr-string.cpp

Index: clang/test/SemaCXX/constexpr-string.cpp
===
--- clang/test/SemaCXX/constexpr-string.cpp
+++ clang/test/SemaCXX/constexpr-string.cpp
@@ -7,7 +7,7 @@
 // RUN: %clang_cc1 %s -triple armebv7-unknown-linux -std=c++2a -fsyntax-only -verify -pedantic -Wno-vla-extension -fno-signed-char
 // RUN: %clang_cc1 %s -triple armebv7-unknown-linux -std=c++2a -fsyntax-only -verify -pedantic -Wno-vla-extension -fno-wchar -DNO_PREDEFINED_WCHAR_T
 
-# 9 "/usr/include/string.h" 1 3 4
+# 9 "/usr/include/string.h" 1 3 4  // expected-warning {{this style of line directive is a GNU extension}}
 extern "C" {
   typedef decltype(sizeof(int)) size_t;
 
@@ -29,7 +29,7 @@
 }
 # 25 "SemaCXX/constexpr-string.cpp" 2
 
-# 27 "/usr/include/wchar.h" 1 3 4
+# 27 "/usr/include/wchar.h" 1 3 4  // expected-warning {{this style of line directive is a GNU extension}}
 extern "C" {
 #if NO_PREDEFINED_WCHAR_T
   typedef decltype(L'0') wchar_t;
Index: clang/test/Preprocessor/line-directive.c
===
--- clang/test/Preprocessor/line-directive.c
+++ clang/test/Preprocessor/line-directive.c
@@ -9,8 +9,8 @@
 # 20 "" 2
 
 // a push/pop before any other line control
-# 10 "enter-0" 1
-# 11 "" 2 // pop to main file
+# 10 "enter-0" 1 // expected-warning {{this style of line directive is a GNU extension}}
+# 11 "" 2 // pop to main file: expected-warning {{this style of line directive is a GNU extension}}
 #error MAIN7
 // expected-error@-1{{MAIN7}}
 
@@ -27,13 +27,15 @@
 #define A 42 "foo"
 #line A
 
-# 42
-# 42 "foo"
+# 42 // expected-warning {{this style of line directive is a GNU extension}}
+# 42 "foo" // expected-warning {{this style of line directive is a GNU extension}}
 # 42 "foo" 2 // expected-error {{invalid line marker flag '2': cannot pop empty include stack}}
+// The next two lines do not get diagnosed because they are considered to be
+// within the system header, where diagnostics are suppressed.
 # 42 "foo" 1 3  // enter
 # 42 "foo" 2 3  // exit
 # 42 "foo" 2 3 4 // expected-error {{invalid line marker flag '2': cannot pop empty include stack}}
-# 42 "foo" 3 4
+# 42 "foo" 3 4 // expected-warning {{this style of line directive is a GNU extension}}
 
 # 'a'// expected-error {{invalid preprocessing directive}}
 # 42 'f' // expected-error {{invalid filename for line marker directive}}
@@ -54,7 +56,7 @@
 
 // Verify that linemarker diddling of the system header flag works.
 
-# 192 "glomp.h" // not a system header.
+# 192 "glomp.h" // not a system header.: expected-warning {{this style of line directive is a GNU extension}}
 typedef int x;  // expected-note {{previous definition is here}}
 typedef int x;  // expected-warning {{redefinition of typedef 'x' is a C11 feature}}
 
@@ -97,7 +99,7 @@
 #line 010  // expected-warning {{#line directive interprets number as decimal, not octal}}
 extern int array[__LINE__ == 10 ? 1:-1];
 
-# 020  // expected-warning {{GNU line marker directive interprets number as decimal, not octal}}
+# 020  // expected-warning {{GNU line marker directive interprets number as decimal, not octal}} expected-warning {{this style of line directive is a GNU extension}}
 extern int array_gnuline[__LINE__ == 20 ? 1:-1];
 
 /* PR3917 */
@@ -106,7 +108,7 @@
 _\
 _LINE__ == 42 ? 1: -1];  /* line marker is location of first _ */
 
-# 51
+# 51 // expected-warning {{this style of line directive is a GNU extension}}
 extern char array2_gnuline[\
 _\
 _LINE__ == 52 ? 1: -1];  /* line marker is location of first _ */
@@ -115,12 +117,12 @@
 #line 0 "line-directive.c" // expected-warning {{#line directive with zero argument is a GNU extension}}
 undefined t; // expected-error {{unknown type name 'undefined'}}
 
-# 115 "main"
-# 116 "enter-1" 1
-# 117 "enter-2" 1
-# 118 "" 2 // pop to enter-1
+# 115 "main" // expected-warning {{this style of line directive is a GNU extension}}
+# 116 "enter-1" 1 // expected-warning {{this style of line directive is a GNU extension}}
+# 117 "enter-2" 1 // expected-warning {{this style of line directive is a GNU extension}}
+# 118 "" 2 // pop to enter-1: expected-warning {{this style of line directive is a GNU extension}}
 #error ENTER1
 // expected-error@-1{{ENTER1}}
-# 121 "" 2 // pop to "main"
+# 121 "" 2 // pop to "main": expected-warning {{this style of line directive is a GNU extension}}
 #error MAIN2
 // 

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Ken Matsui via Phabricator via cfe-commits]

ken-matsui added inline comments.



Comment at: clang/test/Preprocessor/line-directive-system-headers.c:2-6
-// RUN: not %clang_cc1 -E %s 2>&1 | grep 'blonk.c:92:2: error: ABC'
-// RUN: not %clang_cc1 -E %s 2>&1 | grep 'blonk.c:93:2: error: DEF'
-// RUN: not %clang_cc1 -E %s 2>&1 | grep 'line-directive.c:11:2: error: MAIN7'
-// RUN: not %clang_cc1 -E %s 2>&1 | grep 'enter-1:118:2: error: ENTER1'
-// RUN: not %clang_cc1 -E %s 2>&1 | grep 'main:121:2: error: MAIN2'

aaron.ballman wrote:
> ken-matsui wrote:
> > aaron.ballman wrote:
> > > I think we're losing a bunch of test coverage from this. Why did you drop 
> > > these?
> > These are also tested on `line-directive.c` and possibly redundant, so I 
> > removed them.
> > Should I keep them?
> You're right, those are redundant; I'm fine removing them here.
I see :+1:


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/test/Preprocessor/line-directive-system-headers.c:2-6
-// RUN: not %clang_cc1 -E %s 2>&1 | grep 'blonk.c:92:2: error: ABC'
-// RUN: not %clang_cc1 -E %s 2>&1 | grep 'blonk.c:93:2: error: DEF'
-// RUN: not %clang_cc1 -E %s 2>&1 | grep 'line-directive.c:11:2: error: MAIN7'
-// RUN: not %clang_cc1 -E %s 2>&1 | grep 'enter-1:118:2: error: ENTER1'
-// RUN: not %clang_cc1 -E %s 2>&1 | grep 'main:121:2: error: MAIN2'

ken-matsui wrote:
> aaron.ballman wrote:
> > I think we're losing a bunch of test coverage from this. Why did you drop 
> > these?
> These are also tested on `line-directive.c` and possibly redundant, so I 
> removed them.
> Should I keep them?
You're right, those are redundant; I'm fine removing them here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Ken Matsui via Phabricator via cfe-commits]

ken-matsui added a comment.

Thank you for your review!




Comment at: clang/test/Preprocessor/line-directive-system-headers.c:2-6
-// RUN: not %clang_cc1 -E %s 2>&1 | grep 'blonk.c:92:2: error: ABC'
-// RUN: not %clang_cc1 -E %s 2>&1 | grep 'blonk.c:93:2: error: DEF'
-// RUN: not %clang_cc1 -E %s 2>&1 | grep 'line-directive.c:11:2: error: MAIN7'
-// RUN: not %clang_cc1 -E %s 2>&1 | grep 'enter-1:118:2: error: ENTER1'
-// RUN: not %clang_cc1 -E %s 2>&1 | grep 'main:121:2: error: MAIN2'

aaron.ballman wrote:
> I think we're losing a bunch of test coverage from this. Why did you drop 
> these?
These are also tested on `line-directive.c` and possibly redundant, so I 
removed them.
Should I keep them?



Comment at: clang/test/Preprocessor/line-directive-system-headers.c:57-58
+# 192 "glomp.h" 3 // System header.: expected-warning {{this style of line 
directive is a GNU extension}}
+typedef int y;  // expected-note {{previous definition is here}}
+typedef int y;  // expected-warning {{redefinition of typedef 'y' is a C11 
feature}}
 

aaron.ballman wrote:
> It took me a moment, but we get these diagnostics because we asked for 
> warnings in a system header. So I think we're losing test coverage from this 
> change -- previously the test was checking that we suppressed the diagnostics 
> as expected.
> 
> I think you should have two RUN lines, one with `-Wsystem-headers` and one 
> without, so you can test the behavior of both situations. Note, you can do 
> `-verify=system` on the one with `-Wsystem-headers` so that you can write `// 
> system-warning {{whatever}}` on the diagnostics expected to only appear from 
> that RUN line.
> 
> Alternatively, you could make this test solely about system header behavior 
> and modify line-directive.c to not do anything with system headers. But I 
> think that's a much more invasive change.
Thank you! I'm going to edit this file, not `line-directive.c`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thanks, I think we're getting pretty close!




Comment at: clang/test/Preprocessor/line-directive-system-headers.c:2-6
-// RUN: not %clang_cc1 -E %s 2>&1 | grep 'blonk.c:92:2: error: ABC'
-// RUN: not %clang_cc1 -E %s 2>&1 | grep 'blonk.c:93:2: error: DEF'
-// RUN: not %clang_cc1 -E %s 2>&1 | grep 'line-directive.c:11:2: error: MAIN7'
-// RUN: not %clang_cc1 -E %s 2>&1 | grep 'enter-1:118:2: error: ENTER1'
-// RUN: not %clang_cc1 -E %s 2>&1 | grep 'main:121:2: error: MAIN2'

I think we're losing a bunch of test coverage from this. Why did you drop these?



Comment at: clang/test/Preprocessor/line-directive-system-headers.c:57-58
+# 192 "glomp.h" 3 // System header.: expected-warning {{this style of line 
directive is a GNU extension}}
+typedef int y;  // expected-note {{previous definition is here}}
+typedef int y;  // expected-warning {{redefinition of typedef 'y' is a C11 
feature}}
 

It took me a moment, but we get these diagnostics because we asked for warnings 
in a system header. So I think we're losing test coverage from this change -- 
previously the test was checking that we suppressed the diagnostics as expected.

I think you should have two RUN lines, one with `-Wsystem-headers` and one 
without, so you can test the behavior of both situations. Note, you can do 
`-verify=system` on the one with `-Wsystem-headers` so that you can write `// 
system-warning {{whatever}}` on the diagnostics expected to only appear from 
that RUN line.

Alternatively, you could make this test solely about system header behavior and 
modify line-directive.c to not do anything with system headers. But I think 
that's a much more invasive change.



Comment at: clang/test/Preprocessor/line-directive.c:33-34
 # 42 "foo" 2 // expected-error {{invalid line marker flag '2': cannot pop 
empty include stack}}
 # 42 "foo" 1 3  // enter
 # 42 "foo" 2 3  // exit
 # 42 "foo" 2 3 4 // expected-error {{invalid line marker flag '2': cannot pop 
empty include stack}}




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Ken Matsui via Phabricator via cfe-commits]

ken-matsui marked an inline comment as not done.
ken-matsui added inline comments.



Comment at: clang/test/Preprocessor/line-directive.c:33
 # 42 "foo" 2 // expected-error {{invalid line marker flag '2': cannot pop 
empty include stack}}
 # 42 "foo" 1 3  // enter
 # 42 "foo" 2 3  // exit

aaron.ballman wrote:
> ken-matsui wrote:
> > aaron.ballman wrote:
> > > Something is still not quite correct -- these should also be diagnosed as 
> > > an extension (it's the same feature just with flags).
> > This didn't cause a warning on this test file, but another test file I 
> > created caused a warning.
> > 
> > ```
> > // RUN: %clang_cc1 -std=c99 -fsyntax-only -verify -pedantic %s
> > 
> > # 42 "foo" 1 3  // enter: expected-warning {{this style of line directive 
> > is a GNU extension}}
> > ```
> Oooo, wait a tick! This is entering a system header! Perhaps we're 
> silencing the warning because it's "in" a system header!
> 
> That's a neat edge case for us to think about -- do users of this diagnostic 
> *expect* such a construct to be diagnosed? What about the exit from the 
> system header? GCC seems to diagnose the entrance to the system header, but 
> not the exit: https://godbolt.org/z/PKGd4jh64 and I don't know if we want to 
> follow their behavior or not. Our current behavior is defensible, if it marks 
> the entrance to a system header or the exit from a system header, we're 
> silent. User who want to see the system header diagnostics can use 
> `-Wsystem-headers` to get them. So I think I've talked myself into the 
> current behavior here being correct -- but we should probably add a RUN line 
> that enables diagnostics in system headers to show our behavior there.
Ah, I'm quite not familiar with a preprocessor, so I couldn't have understood 
what was going on. Thank you for your clear explanation.

I've added a new file that tests with `-Wsystem-headers`. This test shows that 
it seems mostly correct expectations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Ken Matsui via Phabricator via cfe-commits]

ken-matsui updated this revision to Diff 428301.
ken-matsui added a comment.

Add a test file that uses the `-Wsystem-headers` option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticLexKinds.td
  clang/lib/Lex/PPDirectives.cpp
  clang/test/Preprocessor/line-directive-system-headers.c
  clang/test/Preprocessor/line-directive.c
  clang/test/SemaCXX/constexpr-string.cpp

Index: clang/test/SemaCXX/constexpr-string.cpp
===
--- clang/test/SemaCXX/constexpr-string.cpp
+++ clang/test/SemaCXX/constexpr-string.cpp
@@ -7,7 +7,7 @@
 // RUN: %clang_cc1 %s -triple armebv7-unknown-linux -std=c++2a -fsyntax-only -verify -pedantic -Wno-vla-extension -fno-signed-char
 // RUN: %clang_cc1 %s -triple armebv7-unknown-linux -std=c++2a -fsyntax-only -verify -pedantic -Wno-vla-extension -fno-wchar -DNO_PREDEFINED_WCHAR_T
 
-# 9 "/usr/include/string.h" 1 3 4
+# 9 "/usr/include/string.h" 1 3 4  // expected-warning {{this style of line directive is a GNU extension}}
 extern "C" {
   typedef decltype(sizeof(int)) size_t;
 
@@ -29,7 +29,7 @@
 }
 # 25 "SemaCXX/constexpr-string.cpp" 2
 
-# 27 "/usr/include/wchar.h" 1 3 4
+# 27 "/usr/include/wchar.h" 1 3 4  // expected-warning {{this style of line directive is a GNU extension}}
 extern "C" {
 #if NO_PREDEFINED_WCHAR_T
   typedef decltype(L'0') wchar_t;
Index: clang/test/Preprocessor/line-directive.c
===
--- clang/test/Preprocessor/line-directive.c
+++ clang/test/Preprocessor/line-directive.c
@@ -9,8 +9,8 @@
 # 20 "" 2
 
 // a push/pop before any other line control
-# 10 "enter-0" 1
-# 11 "" 2 // pop to main file
+# 10 "enter-0" 1 // expected-warning {{this style of line directive is a GNU extension}}
+# 11 "" 2 // pop to main file: expected-warning {{this style of line directive is a GNU extension}}
 #error MAIN7
 // expected-error@-1{{MAIN7}}
 
@@ -27,13 +27,13 @@
 #define A 42 "foo"
 #line A
 
-# 42
-# 42 "foo"
+# 42 // expected-warning {{this style of line directive is a GNU extension}}
+# 42 "foo" // expected-warning {{this style of line directive is a GNU extension}}
 # 42 "foo" 2 // expected-error {{invalid line marker flag '2': cannot pop empty include stack}}
 # 42 "foo" 1 3  // enter
 # 42 "foo" 2 3  // exit
 # 42 "foo" 2 3 4 // expected-error {{invalid line marker flag '2': cannot pop empty include stack}}
-# 42 "foo" 3 4
+# 42 "foo" 3 4 // expected-warning {{this style of line directive is a GNU extension}}
 
 # 'a'// expected-error {{invalid preprocessing directive}}
 # 42 'f' // expected-error {{invalid filename for line marker directive}}
@@ -54,7 +54,7 @@
 
 // Verify that linemarker diddling of the system header flag works.
 
-# 192 "glomp.h" // not a system header.
+# 192 "glomp.h" // not a system header.: expected-warning {{this style of line directive is a GNU extension}}
 typedef int x;  // expected-note {{previous definition is here}}
 typedef int x;  // expected-warning {{redefinition of typedef 'x' is a C11 feature}}
 
@@ -97,7 +97,7 @@
 #line 010  // expected-warning {{#line directive interprets number as decimal, not octal}}
 extern int array[__LINE__ == 10 ? 1:-1];
 
-# 020  // expected-warning {{GNU line marker directive interprets number as decimal, not octal}}
+# 020  // expected-warning {{GNU line marker directive interprets number as decimal, not octal}} expected-warning {{this style of line directive is a GNU extension}}
 extern int array_gnuline[__LINE__ == 20 ? 1:-1];
 
 /* PR3917 */
@@ -106,7 +106,7 @@
 _\
 _LINE__ == 42 ? 1: -1];  /* line marker is location of first _ */
 
-# 51
+# 51 // expected-warning {{this style of line directive is a GNU extension}}
 extern char array2_gnuline[\
 _\
 _LINE__ == 52 ? 1: -1];  /* line marker is location of first _ */
@@ -115,12 +115,12 @@
 #line 0 "line-directive.c" // expected-warning {{#line directive with zero argument is a GNU extension}}
 undefined t; // expected-error {{unknown type name 'undefined'}}
 
-# 115 "main"
-# 116 "enter-1" 1
-# 117 "enter-2" 1
-# 118 "" 2 // pop to enter-1
+# 115 "main" // expected-warning {{this style of line directive is a GNU extension}}
+# 116 "enter-1" 1 // expected-warning {{this style of line directive is a GNU extension}}
+# 117 "enter-2" 1 // expected-warning {{this style of line directive is a GNU extension}}
+# 118 "" 2 // pop to enter-1: expected-warning {{this style of line directive is a GNU extension}}
 #error ENTER1
 // expected-error@-1{{ENTER1}}
-# 121 "" 2 // pop to "main"
+# 121 "" 2 // pop to "main": expected-warning {{this style of line directive is a GNU extension}}
 #error MAIN2
 // expected-error@-1{{MAIN2}}
Index: clang/test/Preprocessor/line-directive-system-headers.c

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/test/Preprocessor/line-directive.c:33
 # 42 "foo" 2 // expected-error {{invalid line marker flag '2': cannot pop 
empty include stack}}
 # 42 "foo" 1 3  // enter
 # 42 "foo" 2 3  // exit

ken-matsui wrote:
> aaron.ballman wrote:
> > Something is still not quite correct -- these should also be diagnosed as 
> > an extension (it's the same feature just with flags).
> This didn't cause a warning on this test file, but another test file I 
> created caused a warning.
> 
> ```
> // RUN: %clang_cc1 -std=c99 -fsyntax-only -verify -pedantic %s
> 
> # 42 "foo" 1 3  // enter: expected-warning {{this style of line directive is 
> a GNU extension}}
> ```
Oooo, wait a tick! This is entering a system header! Perhaps we're 
silencing the warning because it's "in" a system header!

That's a neat edge case for us to think about -- do users of this diagnostic 
*expect* such a construct to be diagnosed? What about the exit from the system 
header? GCC seems to diagnose the entrance to the system header, but not the 
exit: https://godbolt.org/z/PKGd4jh64 and I don't know if we want to follow 
their behavior or not. Our current behavior is defensible, if it marks the 
entrance to a system header or the exit from a system header, we're silent. 
User who want to see the system header diagnostics can use `-Wsystem-headers` 
to get them. So I think I've talked myself into the current behavior here being 
correct -- but we should probably add a RUN line that enables diagnostics in 
system headers to show our behavior there.



Comment at: clang/test/Preprocessor/line-directive.c:67-77
 #line 42 "blonk.h"  // doesn't change system headerness.
 
 typedef int z;  // ok
 typedef int z;  // ok
 
 # 97 // doesn't change system headerness.
 

ken-matsui wrote:
> Should these directives also be diagnosed?
Yes, but this may be a case of the directive being in a system header and thus 
the diagnostic is suppressed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Ken Matsui via Phabricator via cfe-commits]

ken-matsui added a comment.

After some investigations, some directives behave weirdly.
I will continue investigating, but do you have any suggestions here?




Comment at: clang/test/Preprocessor/line-directive.c:33
 # 42 "foo" 2 // expected-error {{invalid line marker flag '2': cannot pop 
empty include stack}}
 # 42 "foo" 1 3  // enter
 # 42 "foo" 2 3  // exit

aaron.ballman wrote:
> Something is still not quite correct -- these should also be diagnosed as an 
> extension (it's the same feature just with flags).
This didn't cause a warning on this test file, but another test file I created 
caused a warning.

```
// RUN: %clang_cc1 -std=c99 -fsyntax-only -verify -pedantic %s

# 42 "foo" 1 3  // enter: expected-warning {{this style of line directive is a 
GNU extension}}
```



Comment at: clang/test/Preprocessor/line-directive.c:61
 
 # 192 "glomp.h" 3 // System header.
 typedef int y;  // ok

aaron.ballman wrote:
> This should also be diagnosed.
This directive didn't cause a warning when marking both `# 192 "glomp.h"` and 
this. Marking only either directive works.

Works:
```
...
# 192 "glomp.h" // not a system header.: expected-warning {{this style of line 
directive is a GNU extension}}
typedef int x;  // expected-note {{previous definition is here}}
typedef int x;  // expected-warning {{redefinition of typedef 'x' is a C11 
feature}}

# 192 "glomp.h" 3 // System header.
...
```

Works:
```
...
# 192 "glomp.h" // not a system header.
typedef int x;  // expected-note {{previous definition is here}}
typedef int x;  // expected-warning {{redefinition of typedef 'x' is a C11 
feature}}

# 192 "glomp.h" 3 // System header.: expected-warning {{this style of line 
directive is a GNU extension}}
...
```

Does NOT work:
```
...
# 192 "glomp.h" // not a system header.: expected-warning {{this style of line 
directive is a GNU extension}}
typedef int x;  // expected-note {{previous definition is here}}
typedef int x;  // expected-warning {{redefinition of typedef 'x' is a C11 
feature}}

# 192 "glomp.h" 3 // System header.: expected-warning {{this style of line 
directive is a GNU extension}}
...
```

```
Command Output (stderr):
--
error: 'warning' diagnostics expected but not seen: 
  File /tmp/llvm/llvm-project/clang/test/Preprocessor/line-directive.c Line 
191: this style of line directive is a GNU extension
1 error generated.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Ken Matsui via Phabricator via cfe-commits]

ken-matsui added a comment.

@aaron.ballman

Thank you for your review and sorry to have missed those directives!

I also found additional directives that I might have to diagnose.
Could you please tell me if these are also required?




Comment at: clang/test/Preprocessor/line-directive.c:67-77
 #line 42 "blonk.h"  // doesn't change system headerness.
 
 typedef int z;  // ok
 typedef int z;  // ok
 
 # 97 // doesn't change system headerness.
 

Should these directives also be diagnosed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

It looks like there's still something not quite right because it's warning on 
some instances but not others and I'm not certain why. Can you investigate?




Comment at: clang/test/Preprocessor/line-directive.c:33-34
 # 42 "foo" 2 // expected-error {{invalid line marker flag '2': cannot pop 
empty include stack}}
 # 42 "foo" 1 3  // enter
 # 42 "foo" 2 3  // exit
 # 42 "foo" 2 3 4 // expected-error {{invalid line marker flag '2': cannot pop 
empty include stack}}

Something is still not quite correct -- these should also be diagnosed as an 
extension (it's the same feature just with flags).



Comment at: clang/test/Preprocessor/line-directive.c:61
 
 # 192 "glomp.h" 3 // System header.
 typedef int y;  // ok

This should also be diagnosed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Ken Matsui via Phabricator via cfe-commits]

ken-matsui added a comment.

@aaron.ballman

I could suppress the strange warnings by using `isWrittenInBuiltinFile` and 
`isWrittenInCommandLineFile`. Thank you!
Could you please review this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Ken Matsui via Phabricator via cfe-commits]

ken-matsui updated this revision to Diff 427482.
ken-matsui added a comment.

Update codes as reviewed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticLexKinds.td
  clang/lib/Lex/PPDirectives.cpp
  clang/test/Preprocessor/line-directive.c
  clang/test/SemaCXX/constexpr-string.cpp

Index: clang/test/SemaCXX/constexpr-string.cpp
===
--- clang/test/SemaCXX/constexpr-string.cpp
+++ clang/test/SemaCXX/constexpr-string.cpp
@@ -7,7 +7,7 @@
 // RUN: %clang_cc1 %s -triple armebv7-unknown-linux -std=c++2a -fsyntax-only -verify -pedantic -Wno-vla-extension -fno-signed-char
 // RUN: %clang_cc1 %s -triple armebv7-unknown-linux -std=c++2a -fsyntax-only -verify -pedantic -Wno-vla-extension -fno-wchar -DNO_PREDEFINED_WCHAR_T
 
-# 9 "/usr/include/string.h" 1 3 4
+# 9 "/usr/include/string.h" 1 3 4  // expected-warning {{this style of line directive is a GNU extension}}
 extern "C" {
   typedef decltype(sizeof(int)) size_t;
 
@@ -29,7 +29,7 @@
 }
 # 25 "SemaCXX/constexpr-string.cpp" 2
 
-# 27 "/usr/include/wchar.h" 1 3 4
+# 27 "/usr/include/wchar.h" 1 3 4  // expected-warning {{this style of line directive is a GNU extension}}
 extern "C" {
 #if NO_PREDEFINED_WCHAR_T
   typedef decltype(L'0') wchar_t;
Index: clang/test/Preprocessor/line-directive.c
===
--- clang/test/Preprocessor/line-directive.c
+++ clang/test/Preprocessor/line-directive.c
@@ -9,8 +9,8 @@
 # 20 "" 2
 
 // a push/pop before any other line control
-# 10 "enter-0" 1
-# 11 "" 2 // pop to main file
+# 10 "enter-0" 1 // expected-warning {{this style of line directive is a GNU extension}}
+# 11 "" 2 // pop to main file: expected-warning {{this style of line directive is a GNU extension}}
 #error MAIN7
 // expected-error@-1{{MAIN7}}
 
@@ -27,13 +27,13 @@
 #define A 42 "foo"
 #line A
 
-# 42
-# 42 "foo"
+# 42 // expected-warning {{this style of line directive is a GNU extension}}
+# 42 "foo" // expected-warning {{this style of line directive is a GNU extension}}
 # 42 "foo" 2 // expected-error {{invalid line marker flag '2': cannot pop empty include stack}}
 # 42 "foo" 1 3  // enter
 # 42 "foo" 2 3  // exit
 # 42 "foo" 2 3 4 // expected-error {{invalid line marker flag '2': cannot pop empty include stack}}
-# 42 "foo" 3 4
+# 42 "foo" 3 4 // expected-warning {{this style of line directive is a GNU extension}}
 
 # 'a'// expected-error {{invalid preprocessing directive}}
 # 42 'f' // expected-error {{invalid filename for line marker directive}}
@@ -54,7 +54,7 @@
 
 // Verify that linemarker diddling of the system header flag works.
 
-# 192 "glomp.h" // not a system header.
+# 192 "glomp.h" // not a system header.: expected-warning {{this style of line directive is a GNU extension}}
 typedef int x;  // expected-note {{previous definition is here}}
 typedef int x;  // expected-warning {{redefinition of typedef 'x' is a C11 feature}}
 
@@ -97,7 +97,7 @@
 #line 010  // expected-warning {{#line directive interprets number as decimal, not octal}}
 extern int array[__LINE__ == 10 ? 1:-1];
 
-# 020  // expected-warning {{GNU line marker directive interprets number as decimal, not octal}}
+# 020  // expected-warning {{GNU line marker directive interprets number as decimal, not octal}} expected-warning {{this style of line directive is a GNU extension}}
 extern int array_gnuline[__LINE__ == 20 ? 1:-1];
 
 /* PR3917 */
@@ -106,7 +106,7 @@
 _\
 _LINE__ == 42 ? 1: -1];  /* line marker is location of first _ */
 
-# 51
+# 51 // expected-warning {{this style of line directive is a GNU extension}}
 extern char array2_gnuline[\
 _\
 _LINE__ == 52 ? 1: -1];  /* line marker is location of first _ */
@@ -115,12 +115,12 @@
 #line 0 "line-directive.c" // expected-warning {{#line directive with zero argument is a GNU extension}}
 undefined t; // expected-error {{unknown type name 'undefined'}}
 
-# 115 "main"
-# 116 "enter-1" 1
-# 117 "enter-2" 1
-# 118 "" 2 // pop to enter-1
+# 115 "main" // expected-warning {{this style of line directive is a GNU extension}}
+# 116 "enter-1" 1 // expected-warning {{this style of line directive is a GNU extension}}
+# 117 "enter-2" 1 // expected-warning {{this style of line directive is a GNU extension}}
+# 118 "" 2 // pop to enter-1: expected-warning {{this style of line directive is a GNU extension}}
 #error ENTER1
 // expected-error@-1{{ENTER1}}
-# 121 "" 2 // pop to "main"
+# 121 "" 2 // pop to "main": expected-warning {{this style of line directive is a GNU extension}}
 #error MAIN2
 // expected-error@-1{{MAIN2}}
Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -1431,6 +1431,7 @@
   // 

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Ken Matsui via Phabricator via cfe-commits]

ken-matsui added a comment.

In D124534#3490732 , @aaron.ballman 
wrote:

> The tests run in -cc1 mode and don't #include anything, so I don't think the 
> issue is an internally included SDK. I think the issue could be from this: 
> https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/InitPreprocessor.cpp#L1355
>  and 
> https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/InitPreprocessor.cpp#L1368.
>  You may have to hook up to a debugger to see why we're issuing those 
> surprising warnings. If it turns out that it's these inserted directives, you 
> may have to look at the source location of the digit token to see if 
> `isWrittenInBuiltinFile()` is true or not (and we may need to also check 
> `isWrittenInScratchSpace()` as well, perhaps).

Thank you for the information! I'm going to look into the source code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D124534#3490098 , @ken-matsui 
wrote:

> @aaron.ballman
>
>> If so, I think putting Diag after the call of this function would be better.
>
> With the above change, I tried to add comments to failed tests, but there 
> were over 300 files.
>
> During my investigation, I found most tests printed warnings without file 
> information strangely.
>
>   error: 'warning' diagnostics seen but not expected: 
> Line 0: this style of line directive is a GNU extension
> Line 0: this style of line directive is a GNU extension
>   2 errors generated.

That is rather unexpected.

> If warnings have file information, it should be like:
>
>   error: 'warning' diagnostics seen but not expected: 
> File /tmp/llvm-project/clang/test/Preprocessor/line-directive.c Line 9: 
> this style of line directive is a GNU extension
>   1 error generated.
>
> Even tests that completely do not use preprocessor directives, such as 
> `clang/test/SemaCXX/matrix-type.cpp`, failed with the above strange warnings.
>
> Thus, I suspect that the warnings without file paths have come from 
> internally included SDK (I'm using macOS that includes 
> `/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.3.sdk`)
>  or something that is not related to target test files.
>
> What do you think?

The tests run in -cc1 mode and don't #include anything, so I don't think the 
issue is an internally included SDK. I think the issue could be from this: 
https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/InitPreprocessor.cpp#L1355
 and 
https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/InitPreprocessor.cpp#L1368.
 You may have to hook up to a debugger to see why we're issuing those 
surprising warnings. If it turns out that it's these inserted directives, you 
may have to look at the source location of the digit token to see if 
`isWrittenInBuiltinFile()` is true or not (and we may need to also check 
`isWrittenInScratchSpace()` as well, perhaps).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Ken Matsui via Phabricator via cfe-commits]

ken-matsui added a comment.

@aaron.ballman

> If so, I think putting Diag after the call of this function would be better.

With the above change, I tried to add comments to failed tests, but there were 
over 300 files.

During my investigation, I found most tests printed warnings without file 
information strangely.

  error: 'warning' diagnostics seen but not expected: 
Line 0: this style of line directive is a GNU extension
Line 0: this style of line directive is a GNU extension
  2 errors generated.

If warnings have file information, it should be like:

  error: 'warning' diagnostics seen but not expected: 
File /tmp/llvm-project/clang/test/Preprocessor/line-directive.c Line 9: 
this style of line directive is a GNU extension
  1 error generated.

Even tests that completely do not use preprocessor directives, such as 
`clang/test/SemaCXX/matrix-type.cpp`, failed with the above strange warnings.

Thus, I suspect that the warnings without file paths have come from internally 
included SDK (I'm using macOS that includes 
`/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.3.sdk`)
 or something that is not related to target test files.

What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Ken Matsui via Phabricator via cfe-commits]

ken-matsui added inline comments.



Comment at: clang/lib/Lex/PPDirectives.cpp:1356
+
+PP.Diag(FlagTok, diag::ext_pp_gnu_line_directive);
   } else if (FlagVal == 2) {

aaron.ballman wrote:
> ken-matsui wrote:
> > aaron.ballman wrote:
> > > ken-matsui wrote:
> > > > aaron.ballman wrote:
> > > > > I speculate that this change is wrong.
> > > > > 
> > > > > The goal here is to diagnose any time there's a GNU line marker and 
> > > > > now we're only trigging the diagnostic when the line marker's value 
> > > > > is 1; that misses diagnostics when the marker value is something else.
> > > > > 
> > > > > That's why I suggested warning each place we return `false` from this 
> > > > > function -- those are the situations when the line marker is 
> > > > > syntactically correct and we're going to make use of it in the 
> > > > > caller. (We don't want to warn about use of a line marker when we're 
> > > > > going to generate an error anyway.)
> > > > @aaron.ballman 
> > > > 
> > > > Thank you!
> > > > 
> > > > Just to confirm, do I need to remove the call of `Diag` after 
> > > > `GetLineValue` and put `Diag`s into all branches of returning `false` 
> > > > in this function?
> > > > If so, I think putting `Diag` after the call of this function would be 
> > > > better.
> > > > If so, I think putting Diag after the call of this function would be 
> > > > better.
> > > 
> > > You are correct and I agree, good suggestion!
> > @aaron.ballman 
> > Thank you for your response!
> > 
> > I've updated the code as mentioned, but a bunch of other tests with the 
> > `-pedantic` option failed as the following warnings:
> > 
> > ```
> >  TEST 'Clang :: CXX/expr/expr.const/p2-0x.cpp' FAILED 
> > 
> > Script:
> > --
> > : 'RUN: at line 1';   /tmp/llvm/llvm-project/build/bin/clang -cc1 
> > -internal-isystem /tmp/llvm/llvm-project/build/lib/clang/15.0.0/include 
> > -nostdsysteminc -fsyntax-only -std=c++11 -pedantic -verify=expected,cxx11 
> > -fcxx-exceptions 
> > /tmp/llvm/llvm-project/clang/test/CXX/expr/expr.const/p2-0x.cpp 
> > -fconstexpr-depth 128 -triple i686-pc-linux-gnu
> > : 'RUN: at line 2';   /tmp/llvm/llvm-project/build/bin/clang -cc1 
> > -internal-isystem /tmp/llvm/llvm-project/build/lib/clang/15.0.0/include 
> > -nostdsysteminc -fsyntax-only -std=c++2a -pedantic -verify=expected,cxx20 
> > -fcxx-exceptions 
> > /tmp/llvm/llvm-project/clang/test/CXX/expr/expr.const/p2-0x.cpp 
> > -fconstexpr-depth 128 -triple i686-pc-linux-gnu
> > --
> > Exit Code: 1
> > 
> > Command Output (stderr):
> > --
> > error: 'warning' diagnostics seen but not expected: 
> >   Line 0: this style of line directive is a GNU extension
> >   Line 0: this style of line directive is a GNU extension
> > 2 errors generated.
> > 
> > ...
> > ```
> > 
> > I personally think it would be preferable if the only change of tests would 
> > be `line-directive.c`.
> > So, how about reducing `Diag` calls until the warning doesn't spill over 
> > into other tests?
> > I personally think it would be preferable if the only change of tests would 
> > be line-directive.c.
> > So, how about reducing Diag calls until the warning doesn't spill over into 
> > other tests?
> 
> No, this is expected. We're adding a diagnostic where there wasn't one 
> previously, so some files are going to get caught by that. You can either add 
> the `// expected-warning {{}}` comments to those lines, or if the test has a 
> lot of those lines but isn't really specific to line markers (it just happens 
> to use them to test other functionality) you can disable the diagnostic for 
> that test with `-Wno-gnu-line-marker`.
Ah, I see. I'm going to work on it. Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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

[PATCH] D124534: [clang] Add a diagnostic for line directive of a gnu extension

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Lex/PPDirectives.cpp:1356
+
+PP.Diag(FlagTok, diag::ext_pp_gnu_line_directive);
   } else if (FlagVal == 2) {

ken-matsui wrote:
> aaron.ballman wrote:
> > ken-matsui wrote:
> > > aaron.ballman wrote:
> > > > I speculate that this change is wrong.
> > > > 
> > > > The goal here is to diagnose any time there's a GNU line marker and now 
> > > > we're only trigging the diagnostic when the line marker's value is 1; 
> > > > that misses diagnostics when the marker value is something else.
> > > > 
> > > > That's why I suggested warning each place we return `false` from this 
> > > > function -- those are the situations when the line marker is 
> > > > syntactically correct and we're going to make use of it in the caller. 
> > > > (We don't want to warn about use of a line marker when we're going to 
> > > > generate an error anyway.)
> > > @aaron.ballman 
> > > 
> > > Thank you!
> > > 
> > > Just to confirm, do I need to remove the call of `Diag` after 
> > > `GetLineValue` and put `Diag`s into all branches of returning `false` in 
> > > this function?
> > > If so, I think putting `Diag` after the call of this function would be 
> > > better.
> > > If so, I think putting Diag after the call of this function would be 
> > > better.
> > 
> > You are correct and I agree, good suggestion!
> @aaron.ballman 
> Thank you for your response!
> 
> I've updated the code as mentioned, but a bunch of other tests with the 
> `-pedantic` option failed as the following warnings:
> 
> ```
>  TEST 'Clang :: CXX/expr/expr.const/p2-0x.cpp' FAILED 
> 
> Script:
> --
> : 'RUN: at line 1';   /tmp/llvm/llvm-project/build/bin/clang -cc1 
> -internal-isystem /tmp/llvm/llvm-project/build/lib/clang/15.0.0/include 
> -nostdsysteminc -fsyntax-only -std=c++11 -pedantic -verify=expected,cxx11 
> -fcxx-exceptions 
> /tmp/llvm/llvm-project/clang/test/CXX/expr/expr.const/p2-0x.cpp 
> -fconstexpr-depth 128 -triple i686-pc-linux-gnu
> : 'RUN: at line 2';   /tmp/llvm/llvm-project/build/bin/clang -cc1 
> -internal-isystem /tmp/llvm/llvm-project/build/lib/clang/15.0.0/include 
> -nostdsysteminc -fsyntax-only -std=c++2a -pedantic -verify=expected,cxx20 
> -fcxx-exceptions 
> /tmp/llvm/llvm-project/clang/test/CXX/expr/expr.const/p2-0x.cpp 
> -fconstexpr-depth 128 -triple i686-pc-linux-gnu
> --
> Exit Code: 1
> 
> Command Output (stderr):
> --
> error: 'warning' diagnostics seen but not expected: 
>   Line 0: this style of line directive is a GNU extension
>   Line 0: this style of line directive is a GNU extension
> 2 errors generated.
> 
> ...
> ```
> 
> I personally think it would be preferable if the only change of tests would 
> be `line-directive.c`.
> So, how about reducing `Diag` calls until the warning doesn't spill over into 
> other tests?
> I personally think it would be preferable if the only change of tests would 
> be line-directive.c.
> So, how about reducing Diag calls until the warning doesn't spill over into 
> other tests?

No, this is expected. We're adding a diagnostic where there wasn't one 
previously, so some files are going to get caught by that. You can either add 
the `// expected-warning {{}}` comments to those lines, or if the test has a 
lot of those lines but isn't really specific to line markers (it just happens 
to use them to test other functionality) you can disable the diagnostic for 
that test with `-Wno-gnu-line-marker`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534

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