[PATCH] D119162: [Pseudo] Token/TokenStream, PP directive parser.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

comments on preprocessor part.



================
Comment at: clang/include/clang/Tooling/Syntax/Pseudo/Preprocess.h:49
+/// | `                                printf("hello, ");
+/// |-+ Conditional -+ Directive     #ifndef NDEBUG
+/// | |-+ Code                         printf("debug\n");
----------------
is the `-+ Directive` expected?


================
Comment at: clang/include/clang/Tooling/Syntax/Pseudo/Preprocess.h:60
+struct PPStructure {
+  /// A range of code containing no directives.
+  struct Code {
----------------
maybe mention it includes comments.


================
Comment at: clang/lib/Tooling/Syntax/Pseudo/Preprocess.cpp:18
+
+class Parser {
+public:
----------------
I'd use a more specific name, maybe `PPParser`


================
Comment at: clang/lib/Tooling/Syntax/Pseudo/Preprocess.cpp:21
+  explicit Parser(const TokenStream &Code) : Code(Code), Tok(&Code.front()) {}
+  void parse(PPStructure *result) { parse(result, /*TopLevel=*/true); }
+
----------------
nit: result -> Result


================
Comment at: clang/lib/Tooling/Syntax/Pseudo/Preprocess.cpp:26
+  enum class Cond { None, If, Else, End };
+  static Cond classifyDirective(tok::PPKeywordKind kind) {
+    switch (kind) {
----------------
nit: kind => Kind


================
Comment at: clang/lib/Tooling/Syntax/Pseudo/Preprocess.cpp:45
+  // Parses tokens starting at Tok into PP.
+  // If we reach an #end or #else directive that ends PP, returns it.
+  // If TopLevel is true, then we do not expect #end and always return None.
----------------
#end => #endif.


================
Comment at: clang/lib/Tooling/Syntax/Pseudo/Preprocess.cpp:60
+    while (Tok->Kind != tok::eof) {
+      while (StartsDirective()) {
+        PPStructure::Directive Directive;
----------------
is there any special reason to use a nested while? it looks like we can replace with an `if`. -- in each iteration, we get a Chunk result (either a Code, or a Directive), something like below should probably work?

```
while (Tok->Kind != tok::eof) {
  if (!StartsDirective()) {
     // handle Chunk::Code case
     continue;
  } else {
     // handle directive case.
  }
}
```




================
Comment at: clang/lib/Tooling/Syntax/Pseudo/Preprocess.cpp:77
+        }
+        assert(Kind == Cond::Else || Kind == Cond::End);
+        return std::move(Directive);
----------------
It took me a while to understand the logic here. It feels more natural to make this case as an early-exit case, and leave the above general case as a default case like 

```
if (ParsingCondBranch && (Kind == Cond::Else || Kind == Cond::End)
  return ...
```


================
Comment at: clang/lib/Tooling/Syntax/Pseudo/Preprocess.cpp:91
+  // Parse the rest of a conditional section, after seeing the #if directive.
+  // Returns after consuming the #end directive.
+  void parseConditional(PPStructure::Conditional *C) {
----------------
#end => #endif


================
Comment at: clang/lib/Tooling/Syntax/Pseudo/Preprocess.cpp:134
+  const Token *Tok;
+  clang::IdentifierTable Idents;
+};
----------------
maybe rename to PPKeywordTable, it seems clearer for the purpose


================
Comment at: clang/lib/Tooling/Syntax/Pseudo/Preprocess.cpp:145
+
+static llvm::StringLiteral ppKeywordName(tok::PPKeywordKind kind) {
+  switch (kind) {
----------------
nit: Kind


================
Comment at: clang/lib/Tooling/Syntax/Pseudo/Preprocess.cpp:145
+
+static llvm::StringLiteral ppKeywordName(tok::PPKeywordKind kind) {
+  switch (kind) {
----------------
hokein wrote:
> nit: Kind
looks like this function can be lifted to TokenKinds.h file (there is a similar getTokenName there), maybe a FIXME?


================
Comment at: clang/test/Syntax/lex.test:37
+PPS-NEXT: #endif (2 tokens)
+PPS-NEXT: code (1 tokens)
+
----------------
`1 tokens` seems silly :( maybe it doesn't matter.


================
Comment at: clang/test/Syntax/lex.test:38
+PPS-NEXT: code (1 tokens)
+
----------------
Does this extra trailing blank line matter?


================
Comment at: clang/unittests/Tooling/Syntax/Pseudo/PreprocessTest.cpp:40
+
+TEST(PPStructure, Parse) {
+  LangOptions Opts;
----------------
maybe add a mismatched-if testcase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119162