[PATCH] D113795: Comment Sema: Eliminate or factor out DeclInfo inspection (NFC)

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In D113795#3137250 <https://reviews.llvm.org/D113795#3137250>, @gribozavr2 wrote:

> Sorry, here I also find the old code to be more readable.

Admittedly this isn't mainly about readability but about reducing boilerplate. A large number of functions start like this:

  if (!ThisDeclInfo)
    return false;
  if (!ThisDeclInfo->IsFilled)
    inspectThisDecl();

The diffstat is 95 insertions, 208 deletions.

> - I don't see a problem with checks that are only used once. They are encapsulated in functions with meaningful names, making the code more readable. Compare `Sema::checkFunctionDeclVerbatimLine` before and after, for example.

Agreed, though you'll find I've basically only inlined one- or maybe two-liners. Having them in-place reduces the number of visual jumps in the code flow. Maybe `isFunctionPointerVarDecl` is worth preserving though.

> - checkDecl looks like a too complex abstraction for the task at hand: it accepts a function pointer

Perhaps that's an issue with the name? I suggested `hasDeclThat`, then it's clear that this takes a predicate. Also reads almost like a proper sentence: `hasDeclThat(isXXX)`.

> (and it is incorrect for the user to call those other functions directly),

Not quite true, as you can see I'm calling many of these functions directly. We go through `checkDecl` only if we don't know if `ThisDeclInfo` is available.

> it hardcodes the result value of `false` when the decl is not available etc.

Would `hasDeclThat` address that concern?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113795