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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
