[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