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