[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#2055419 <https://reviews.llvm.org/D79963#2055419>, @jroelofs wrote:

> In D79963#2054177 <https://reviews.llvm.org/D79963#2054177>, @jhenderson wrote:
>
> > This will still cause a small number of false positives (e.g. the three llvm-readobj tests would all generate them, I believe), but it would be easier to reorganise the comment to not trip over it without making it look "ugly" and inconsistent with other comments elsewhere. I guess there might be some false negatives, but I doubt there'd be all that many at all?
>
>
> I like that too. I'll prototype this, and see if I can get some concrete numbers on the change in diagnosed cases. Also promise to summarize on llvm-dev (haven't sparked that discussion back up yet since I've been quite busy, but I definitely see the importance / value of it).


After some experimenting, this seems like a pretty decent filter:

  static bool shouldDiagnoseMissingColon(StringRef LineBegin) {
    StringRef Preamble = LineBegin.rtrim(" \t");
  
    if (Preamble.size() == 0)
      return true;
  
    // C++ style line comments.
    if (Preamble.endswith("//"))
      return true;
  
    // C style block comments.
    if (Preamble.endswith("/*"))
      return true;
  
    char Ending = Preamble.back();
  
    // Other forms of line comments.
    if (StringRef("#;*!@&%").count(Ending))
      return true;
  
    return false;
  }

In the `llvm/test` suite, this leaves ~46 tests that trigger on things that are clearly comments about the CHECK directives themselves:

e.g:

  llvm/test/CodeGen/Mips/cconv/arguments.ll:67

A few more cases of missing colons I hadn't seen before (I have a patch to fix these, haven't pushed it yet):

  llvm/test/MC/ARM/ldr-pseudo-cond-darwin.s
  llvm/test/MC/ARM/ldr-pseudo-cond.s
  llvm/test/tools/dsymutil/ARM/scattered.c
  llvm/test/tools/dsymutil/fat-binary-output.test
  llvm/test/tools/llvm-objdump/ELF/ARM/v5te-subarch.s
  llvm/test/tools/llvm-readobj/COFF/codeview-linetables.test
  llvm/test/tools/llvm-symbolizer/flag-grouping.test

And some new false positives on mips tests that use things like `--check-prefix=16` where the diagnostic fires on the llvm value with the same name `%16 = ...` [3].

  llvm/test/CodeGen/Mips/selgt.ll
  llvm/test/CodeGen/Mips/hf16_1.ll

I'm warming up to the idea of using `##` as the way to silence the diagnostic, since it's less invasive than `COM:`. That makes a lot of sense in source languages where `#` is already the comment character, but what about elsewhere? Should we do `@ ##` in assembler, and `// ##` in C, for example?


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

https://reviews.llvm.org/D79963