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

[Jon Roelofs via Phabricator via llvm-commits]

jroelofs added a comment.

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.

I don’t have an exact number but since I started this investigation a couple of months ago, a small handful of cases of it have been added to the test suite... this is an ongoing problem.

> I think people will repeatedly trip over this when they are writing tests,

It may help make things more obvious if the missing colon diagnostic mentions how to work around the issue.

> both new-comers and existing users, because it's not a requirement to use COM in most situations. I think this will mostly lead to annoyance and frustration, and consequently, less of a desire to do any of 1) write comments, 2) write tests, or 3) contribute to LLVM (due to too many hurdles to understand), which in turn will impact the quality of our codebase.

Something we’re a bit too early to see, which I expect we might, is people changing their habits w.r.t. choosing check prefixes & how they refer to them in comments.


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

https://reviews.llvm.org/D79963