[PATCH] D121165: [pseudo] Add crude heuristics to choose taken preprocessor branches.

[Haojian Wu via Phabricator via cfe-commits]

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