[PATCH] D101192: Add support for #elifdef and #elifndef

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 4 inline comments as done.
aaron.ballman added inline comments.


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:754
     break;
+  case tok::pp_elifdef:
+  case tok::pp_elifndef:
----------------
erichkeane wrote:
> Just looking through this, so forgive me if I there is something I don't understand... Why is this not doing something like `parsePPIf(/*IfDef=*/true)` like above?  
I had originally considered that, but it turns out that `parsePPElif()` just calls `parsePPElse()` and doesn't care about the condition whatsoever.


================
Comment at: clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp:924
     case pp_elif:
+    case pp_elifdef:
+    case pp_elifndef:
----------------
erichkeane wrote:
> Hrmph, not sure I understand this part either.
I'm not 100% certain myself.


================
Comment at: clang/lib/Lex/PPDirectives.cpp:642
+
+          CheckEndOfDirective(IsElifDef ? "elifdef" : "elifndef");
+
----------------
erichkeane wrote:
> Can you just pass 'Directive' here?
I think I'd have to pass `Directive.str().c_str()` to ensure I get a null terminated string to pass (`CheckEndOfDirective()` takes a `const char *`), and that involves an allocation, so I think this form may be better? I don't have a strong opinion though.


================
Comment at: clang/lib/Lex/PPDirectives.cpp:3265
+  if (CurPPLexer->popConditionalLevel(CI)) {
+    Diag(DirectiveTok, diag::pp_err_elif_without_if); // FIXME: wrong error
+    return;
----------------
erichkeane wrote:
> Oh? What is the 'right' error here?
Good catch, I'll correct this.


================
Comment at: clang/lib/Lex/PPDirectives.cpp:3274
+  if (CI.FoundElse)
+    Diag(DirectiveTok, diag::pp_err_elif_after_else); // FIXME: wrong error
+
----------------
erichkeane wrote:
> Same here:)?
Same here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101192