<div dir="ltr">Hello, <div><br></div><div>here I have updated the patch with the suggestions you pointed out addressed.</div><div><br></div><div>The only think I skipped is using \brief in the comment of isBinOpWithFlags,because every other function in the file uses the style it is actually uses, so probably I think that keeping everything consistent is quite a good idea.</div>
<div><br></div><div>Let me know what you think!</div><div><br></div><div>Cheers,</div><div>Marcello</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-05 13:49 GMT-07:00 Andrea Di Biagio <span dir="ltr"><<a href="mailto:andrea.dibiagio@gmail.com" target="_blank">andrea.dibiagio@gmail.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">On Thu, Jun 5, 2014 at 8:50 PM, Marcello Maggioni <<a href="mailto:hayarms@gmail.com">hayarms@gmail.com</a>> wrote:<br>

> Hi Andrea,<br>
><br>
> answers are inlined!<br>
><br>
</div>> <snip><br>
<div><div class="h5">>> About patch "flag_nodes_v4.patch"<br>
>><br>
>> In include/llvm/CodeGen/SelectionDAG.h:<br>
>><br>
>> +  /// isBinOpWithFlags - Returns true if the opcode is a binary operation<br>
>> +  /// with flags.<br>
>> +  static bool isBinOpWithFlags(unsigned Opcode) {<br>
>> +    switch (Opcode) {<br>
>> +    case BINOP_NODES_W_FLAGS: return true;<br>
>> +    default: return false;<br>
>> +    }<br>
>> +  }<br>
>> +<br>
>><br>
>> I think you can move that definition in SelectionDAGNodes.h. You can<br>
>> simplify the code in method 'BinaryWithFlagsSDNode::classof' If you<br>
>> can make that definition visible. For example, method '<br>
>> BinaryWithFlagsSDNode ::classof'  could be rewritten as follows:<br>
>><br>
>> //<br>
>>   static bool classof(const SDNode *N) {<br>
>>     return isBinOpWithFlags(N->getOpcode());<br>
>> //<br>
>><br>
>> Also, if possible, please remove macro BINOP_NODES_W_FLAGS. In my<br>
>> opinion it doesn't make the code more readable.<br>
><br>
><br>
> About that ... BINOP_NODES_W_FLAGS is used in a couple of places (in<br>
> switches) where mainly I cannot avoid to have the case labels there (I would<br>
> like to use isBinOpWithFlags() everywhere, but in switches is not possible<br>
> and putting the check outside the switch would be less performant, so I<br>
> prefer having the cases there.<br>
><br>
> Do you think just duplicating the cases in those couple of places would be<br>
> ok?<br>
<br>
</div></div>Yes, that was the idea. I don't think it is bad to have those cases<br>
duplicated in 'AddNodeIDCustom' and 'isBinOpWithFlags' . We already do<br>
something similar for other dag nodes (see for example AtomicSDNode).<br>
That said, your approach is not wrong. It is just that, in general, I<br>
prefer not to use macros when possible.<br>
</blockquote></div><br></div>