aaronpuchert added a comment.
In D113793#3137244 <https://reviews.llvm.org/D113793#3137244>, @gribozavr2
wrote:
> I find the code more readable and robust when the `check*` function checks
> its applicability itself. After this refactoring, it is not so clear when
> each check functions applies, what are the correct conditions to call them.
The current code doesn't really do that either. Wouldn't we have to call
**all** `check*` functions in **all** `actOn*` methods? But we only call a
certain subset of checks which then check additional applicability conditions
internally. Also we do check some conditions before: `actOnBlockCommandFinish`
knows not to call `checkReturnsCommand` and `checkDeprecatedCommand` when there
is no declaration, which requires more knowledge about what the checks do than
to write
if (Info->IsReturnsCommand)
checkReturnsCommand(Command);
if (Info->IsDeprecatedCommand)
checkDeprecatedCommand(Command);
which seems pretty natural.
Now I don't think we want to go though the whole battery for every command that
we encounter, so if we let the `actOn*` methods decide **which** checks to run,
we can also let them decide **when** to run them.
> To ensure correct usage, probably we should be adding equivalent asserts to
> the beginning of each function; at which point I'm not sure if the new code
> is better.
To some extent D113794 <https://reviews.llvm.org/D113794> is doing that. (By
having `llvm_unreachable` in the switches.) In the remaining functions nothing
bad would happen, I think (other than false positive warnings).
> For example, consider `checkContainerDecl`. [...] This code clearly shows the
> condition for the warning: the command is "RecordDetailLike" but the decl is
> not "RecordLike". In the new code this logic is split across two functions.
After the change the call site is
if (Info->IsRecordLikeDetailCommand)
checkContainerDecl(BC);
So it's saying: if we have a command providing the details for a "RecordLike",
let's check if we really have a "RecordLike" there. (Perhaps it should be
`checkRecordLikeDecl`?) Then the check function is doing the check, i.e. "do we
actually have a RecordLike here"?
So the idea is that `actOn*` methods decide which checks to run, while the
check functions can assume they're applicable. Ideally they'd only see the
declaration, but we have to pass the command in for diagnostics.
> Is repeated `CommandInfo` fetching a performance issue? It shouldn't be
> because it is basically address arithmetic on the static array of commands
> using the command ID as an index.
Not that I observed, though in relative terms it could be: take as an extreme
example `actOnBlockCommandFinish` where we don't actually need to run any of
the checks, because we have an unrelated block command. Then we're fetching the
same data four times instead of just once, and not doing anything else.
Compared to the remaining compilation process it's likely not an issue though,
that's right.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113793/new/
https://reviews.llvm.org/D113793
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
