[PATCH] D79963: [llvm][test] Add COM: directives before colon-less non-CHECKs in comments. NFC
Joel E. Denny via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu May 14 17:26:37 PDT 2020
jdenny added a comment.
In D79963#2037615 <https://reviews.llvm.org/D79963#2037615>, @jroelofs wrote:
> In D79963#2037408 <https://reviews.llvm.org/D79963#2037408>, @jdenny wrote:
>
> > This looks like a move in the right direction to me, but I'm biased.
>
>
> Thanks for having a look!
Thanks for working on all this.
>
>
>> Hopefully others will chime in, especially individual test suite maintainers.
>>
>> Here are some minor style suggestions, but feel free to outright ignore them if I'm the only one who cares:
>>
>> 1. Sometimes you add `COM:` only to lines your diagnostic might catch and not to other lines in the same comment block. I'd prefer to see it on every line in a comment block.
>
> I was aiming for the latter, though it looks like the style I ended up with for all of the "here's what each of the CHECKs mean" descriptions having their first comment without the `COM:` directive. Happy to make those consistent with the rest.
Thanks.
>
>
>>
>>
>> 2. Where `;;` was used to differentiate true comments from FileCheck check directives, you now have `;; COM:`. I think `; COM:` is sufficient.
>
> I waffled about that, and even considered adding `;;` as a comment directive in those files. Likewise for `##`. Seems not worth it to add more flavors of `COM:` just for that,
I strongly agree that we should not go down that path. I feel like it's just asking for new dimensions of silent FileCheck prefix mistakes. Let's not deviate from `COM:` except as a last resort.
> and I could go either way on `;; COM:` vs `; COM:`. Happy to update those too.
That's my vote, but I'll leave it up to you and anyone else who might comment. It's a minor point.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79963/new/
https://reviews.llvm.org/D79963
More information about the llvm-commits
mailing list