[PATCH] D113793: Comment Sema: Run checks only when appropriate (NFC)

[Aaron Puchert via Phabricator via cfe-commits]

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