[PATCH] D79963: [llvm][test] Add COM: directives before colon-less non-CHECKs in comments. NFC

Fri May 22 09:38:38 PDT 2020

MaskRay added a subscriber: probinson.
MaskRay added a comment.

In D79963#2050911 <https://reviews.llvm.org/D79963#2050911>, @jroelofs wrote:

> In D79963#2038710 <https://reviews.llvm.org/D79963#2038710>, @jroelofs wrote:
>
> > In D79963#2038058 <https://reviews.llvm.org/D79963#2038058>, @jhenderson wrote:
> >
> > > I saw the benefit initially when the proposal was made, but now seeing the consequence in practice in the tools tests at least makes me realise I'm not a fan of the change to make FileCheck error if it sees the string somewhere else in the line. In particular, GNU and LLVM are common prefixes used to distinguish checks for different output modes, but also will often appear in comments to explain the test cases. Similar comments apply to the machine names etc. Needing to add COM is unobvious, and makes the comments messier to read, I found, as soon as I started reading.
> >
> >
> > Let’s not lose sight of the fact that this whole change is a trade off: we are attempting to trade comment succinctness/obviousness for the ability to diagnose a particularly nasty failure mode of missing colons. This comment thing may not be the right tradeoff, but given how constrained the problem space is, I suspect we will end up sacrificing a bit of intuitiveness somewhere regardless. So long as these new diagnosed-but-unintuitive cases aren’t too frequent/annoying, IMO that’s a better place to be than having room for the other not-diagnosable-and-unintuitive failure mode.
>
>
> Here's what I'm imagining the other patch will do to alleviate this:
>
>   llvm/test/CodeGen/Mips/cconv/arguments.ll:67:3: error: colon required after check directive 'O32'
>   ; O32 starts loading from the stack. The addresses start at 16 because space is
>     ^
>   llvm/test/CodeGen/Mips/cconv/arguments.ll:67:3: note: to silence this diagnostic, prefix the line with a COM: directive
>   ; O32 starts loading from the stack. The addresses start at 16 because space is
>     ^
>
>
> Does that address your un-obviousness concerns, @jhenderson?
>
> Also, how does everyone feel about the relative weight of these two cases of un-obviousness:
>
> 1. Diagnosed syntactic errors for things that were previously ok (i.e. the `note:` above)
> 2. Undiagnosed syntactic mis-use (i.e. the missing colon bug)
>
>   IMO this tradeoff is a net improvement, especially given the volume of (2) I've continuously been fixing.

Thank James for weighing in:) And thanks for the revert. Here are my thoughts:

If a check directive is preceded by another word (e.g. `please CHECK`) there are plenty of tests in this diff), my preference is not to diagnose the potential problem (the false positive rate is very high).

If a check directive is preceded by a predefined punctuation set, it may be a Gotcha A described in "[RFC] Improving FileCheck". Diagnose. Some of touched tests are related.
`COM:` can suppress the diagnostic, but personally I prefer repeating the punctuation, i.e. `## CHECK is a comment` (assembly,yaml,etc) or `;; CHECK is a comment` (.ll) or `/// CHECK` (BCPL/C99/C++)
There are multi-byte comment markers e.g. `/*`. It may not be clear how we should suppress such diagnostics. My thought is that these are probably too few to worry about for the initial implementation.
(I recall that @probinson has a comprehensive list of the predefined punctuation set. Starting with `#` `;` `//` initially can probably cover most of misuses. We can extend the set on demand in the future.)
This preferred commenting approach indeed requires a separate llvm-dev discussion.

================
Comment at: llvm/test/CodeGen/Mips/cconv/arguments-varargs.ll:145
+; COM: Set up the stack with an 8-byte local area. N32/N64 must also make room for
+; COM; the argument save area (56 bytes).
 ; O32:           addiu  [[SP:\$sp]], $sp, -8
----------------
Is `COM;` intentional?

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

https://reviews.llvm.org/D79963