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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
