hokein added inline comments.
================
Comment at: clang/include/clang/Tooling/Syntax/Pseudo/DirectiveMap.h:123
+/// The choices are stored in Conditional::Taken nodes.
+void chooseConditionalBranches(DirectiveMap &, const TokenStream &Code);
+
----------------
off-topic: a random idea on the name of `DirectiveMap` came up to my mind when
reading the code, how about `DirectiveTree`, the comment of it says the
structure is a tree, and the `BranchChooser` is actually a tree-visiting
pattern.
================
Comment at: clang/include/clang/Tooling/Syntax/Pseudo/Token.h:84
+ /// Returns the next token in the stream, skipping over comments.
+ const Token &next_nc() const {
+ const Token *T = this;
----------------
nit: the name doesn't match the LLVM code-style.
================
Comment at: clang/lib/Tooling/Syntax/Pseudo/DirectiveMap.cpp:210
+namespace {
+class BranchChooser {
+public:
----------------
We evaluate each conditional directive *independently*, I wonder whether it is
important to use any low-hanging tricks to ensure consistent choices are made
among different conditional directives. (my gut feeling is that it is nice to
have, but we should not worry too much about it at the moment).
For example,
```
#ifdef __cplusplus
extern "C" {
#endif
....
#ifdef __cplusplus
}
#endif
```
If we enable the first #if, and the second one should be enabled as well. (this
example is a trivial and common pattern, it has been handled by the existing
code).
================
Comment at: clang/lib/Tooling/Syntax/Pseudo/DirectiveMap.cpp:218
+ // Describes the code seen by making particular branch choices.
+ struct Score {
+ unsigned Tokens = 0; // excluding comments and directives
----------------
nit: add a comment saying `higher is better`.
================
Comment at: clang/lib/Tooling/Syntax/Pseudo/DirectiveMap.cpp:225
+ // Seeing errors is bad, other things are good.
+ return std::tie(Other.Errors, Tokens, Directives) >
+ std::tie(Errors, Other.Tokens, Other.Directives);
----------------
Maybe `(-Errors, Tokens, Directives) > (-Other.Errors, ...)`
================
Comment at: clang/lib/Tooling/Syntax/Pseudo/DirectiveMap.cpp:295
+ // Is this the best branch so far? (Including if it's #if 1).
+ if (TookTrivial || !C.Taken || BranchScore > Best) {
+ Best = BranchScore;
----------------
nit: `!C.Taken` => `!C.Token.hasValue()`, which is clearer.
================
Comment at: clang/lib/Tooling/Syntax/Pseudo/DirectiveMap.cpp:305
+ // false if the branch is never taken, and None otherwise.
+ llvm::Optional<bool> isTriviallyTaken(const DirectiveMap::Directive &Dir) {
+ switch (Dir.Kind) {
----------------
nit: instead of using `trivially`, I think `confidently` fits better what the
method does.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121165/new/
https://reviews.llvm.org/D121165
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
