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

Fri Apr 29 05:59:46 PDT 2022

aaron.ballman added a comment.

In D124534#3481264 <https://reviews.llvm.org/D124534#3481264>, @ken-matsui wrote:

> Hi, @aaron.ballman
>
> Thanks for your kind reviews!
> I’ve updated the code, but is this what you’d expected?

It looks like the precommit CI pipeline can't test your patch -- I think you may have uploaded just the changes since last time rather than the full set of changes against the tree. I don't think we're quite there yet, but we're not too far off either, I think. Thank you for continuing to work on this!

================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1114-1116
                             GNUImaginaryConstant, GNUIncludeNext,
                             GNULabelsAsValue, GNUNullPointerArithmetic,
                             GNUPointerArith, RedeclaredClassMember,
----------------
You'll likely also need to re-flow the line to the usual 80-col limit.

================
Comment at: clang/lib/Lex/PPDirectives.cpp:1356
+
+    PP.Diag(FlagTok, diag::ext_pp_gnu_line_directive);
   } else if (FlagVal == 2) {
----------------
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.)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124534