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