[PATCH] D15994: Allow for unfinished #if blocks in preambles.

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

Thanks for working on this! I have some comments below:



================
Comment at: include/clang/Lex/Preprocessor.h:291
+
+    void setState(State s) { ConditionalStackState = s; }
+
----------------
`setState` is redundant IMHO, just assign to `ConditionalStackState` when you use `setState` below.


================
Comment at: include/clang/Lex/Preprocessor.h:301
+
+    const ArrayRef<PPConditionalInfo> getStack() const {
+      return ConditionalStack;
----------------
the `const` is redundant for the return type.


================
Comment at: include/clang/Lex/Preprocessor.h:310
+
+    void setStack(const ArrayRef<PPConditionalInfo> &s) {
+      if (!isRecording() && !isReplaying())
----------------
You can pass `ArrayRef` by value.


================
Comment at: include/clang/Lex/Preprocessor.h:1974
+
+  const ArrayRef<PPConditionalInfo> getPreambleConditionalStack() const
+  { return PreambleConditionalStack.getStack(); }
----------------
the `const` is redundant for the return type.


================
Comment at: include/clang/Lex/Preprocessor.h:1978
+  void setRecordedPreambleConditionalStack(
+      const SmallVector<PPConditionalInfo, 4> &s) {
+    PreambleConditionalStack.setStack(s);
----------------
Please use `ArrayRef` instead of `SmallVector` here and in the function below.


================
Comment at: include/clang/Lex/PreprocessorLexer.h:181
+
+  void setConditionalLevels(const ArrayRef<PPConditionalInfo> &CL)
+  {
----------------
`ArrayRef` can be passed by value.


================
Comment at: include/clang/Lex/PreprocessorOptions.h:87
+  /// When the lexer is done, one of the things that need to be preserved is the
+  /// conditional #if stack, so the ASTWriter/ASTReader can safe/restore it when
+  /// processing the rest of the file.
----------------
Typo: `save`


================
Comment at: lib/Serialization/ASTReader.cpp:2896
+        for (unsigned Idx = 0, N = Record.size() - 1; Idx < N; /* in loop */) {
+          auto loc = ReadSourceLocation(F, Record, Idx);
+          bool WasSkipping = Record[Idx++];
----------------
`Loc` should be capitalized.


================
Comment at: lib/Serialization/ASTWriter.cpp:2269
+  if (PP.isRecordingPreamble() && PP.hasRecordedPreamble()) {
+    for (const auto &Cond : PP.getPreambleConditionalStack()) {
+      AddSourceLocation(Cond.IfLoc, Record);
----------------
I think you should add an assertion here that verifies that the ASTWriter isn't creating a module.


https://reviews.llvm.org/D15994