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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 6 inline comments as done.
sammccall added inline comments.


================
Comment at: clang/lib/Tooling/Syntax/Pseudo/DirectiveMap.cpp:210
+namespace {
+class BranchChooser {
+public:
----------------
hokein wrote:
> 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).
Added a comment describing this case, I don't think we should tackle this now.


================
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) {
----------------
hokein wrote:
> nit: instead of using `trivially`, I think `confidently` fits better what the method does.
You're right this is a bad name, but also because returning false doesn't mean it's not trivially/confidently taken, but rather that it's trivially/confidently *not* taken.
I think removing the adverb actually makes this clearer.

Also this case is confusing
```
#if foo
#else // is this "always taken"?
#bar
```

Renamed to `isTakenWhenReached`, wdyt?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121165