[PATCH] D67544: [TableGen] Support encoding and decoding per-HwMode

[James Molloy via Phabricator via llvm-commits]

jmolloy added a comment.

Hi Roman,

In D67544#1669253 <https://reviews.llvm.org/D67544#1669253>, @lebedev.ri wrote:

> In D67544#1669238 <https://reviews.llvm.org/D67544#1669238>, @jmolloy wrote:
>
> > Hi Roman,
> >
> > What are you referring to?
> >
> >   (a) This diff has not been submitted, it is a patch for code review.
>
>
> Yep!
>
> > If it doesn't have tests, that is something that can be mentioned in this review without being passive-aggressive.
>
> For future reference, how those my two review comments are passive-aggressive?
>  From where //i// stand that remark is.


Your comment was (parsed by me as being) accusatory of submitting code without test coverage. The comment is written to be deliberately unanswerable as you well know the section you linked to says not to submit code without test coverage.

This use of rhetoric will come across as accusatory to some. In future, to avoid misinterpretation, perhaps phrase more lightly; something like "This doesn't look NFC to me, shouldn't it have tests?".

> 
> 
>>   (b) This diff has tests.
> 
> Cool, let's make that more obvious :)
>  Usually in most other cases that wording "NFC for in-tree targets" means something different..

I'm happy to reword the commit message. Note that "NFC for in-tree targets" implies it's not NFC for non-in-tree targets, but I'm very happy to add clarity to the commit message.

In future, to avoid misinterpretation, it might be a good idea to read the patch fully before accusing a submitter of not adding tests.

> 
> 
>> Cheers,
>> 
>> James




Repository:
  rL LLVM

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

https://reviews.llvm.org/D67544