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
