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