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

Jon Roelofs via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri May 22 05:51:27 PDT 2020


jroelofs added a comment.

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.


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

https://reviews.llvm.org/D79963





More information about the llvm-commits mailing list