<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 19, 2014 at 10:28 PM, Michael Ilseman <span dir="ltr"><<a href="mailto:milseman@apple.com" target="_blank">milseman@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class=""><br>
> On Nov 19, 2014, at 10:03 PM, Mehdi Amini <<a href="mailto:mehdi.amini@apple.com">mehdi.amini@apple.com</a>> wrote:<br>
><br>
><br>
>> On Nov 19, 2014, at 8:42 PM, Michael Ilseman <<a href="mailto:milseman@apple.com">milseman@apple.com</a>> wrote:<br>
>><br>
>> The git-diff got a little convoluted in it’s +'s and -'s, so I’ll trust that you kept the functionality the same.<br>
>><br>
>> + /// Don't want to copy this<br>
>> + ConstantComparesGatherer(const ConstantComparesGatherer &) = delete;<br>
>> +<br>
>><br>
>> If that’s the case, you also probably want to delete the assignment operator.<br>
><br>
> It is not that important, it was just to avoid any mistake, but it is a private class. I removed it.<br>
<br>
</span>Sorry, I was unclear. By “delete” I meant the “= delete” pattern. I think having the copy constructor be deleted by adding “= delete” is a good idea. I was saying to also do “ConstantComparesGatherer &operator=(const ConstantComparesGatherer &) = delete;” to get rid of the assignment operator, which like the copy constructor, will get created for you.<br></blockquote><div><br></div><div>Handily enough, C++11 provides the Rule of 4-or-5 in the language now. If you just delete, say, the move constructor, then the move assignment operator won't be provided and the copy ctor and copy assignment operator will be implicitly deleted. Of course one wrinkle to this is that MSVC doesn't implement any of this - but given that this is just to make sure the build breaks when these things are used, we get enough builds on compilers that do support these features that the mistakes won't last long anyway if someone deving on MSVC checks in such a usage.<br><pre style="color:rgb(0,0,0)">$ cat move.cpp
#include <utility>
struct foo {
foo();
foo(foo&&) = delete;
};
foo f;
foo g = std::move(f);
foo h = f;
int main() {
f = std::move(f);
f = f;
}
blaikie@blaikie-linux:/tmp/dbginfo$ clang++-tot -fsyntax-only -std=c++11 move.cpp
move.cpp:8:5: error: call to deleted constructor of 'foo'
foo g = std::move(f);
^ ~~~~~~~~~~~~
move.cpp:4:3: note: 'foo' has been explicitly marked deleted here
foo(foo&&) = delete;
^
move.cpp:9:5: error: call to implicitly-deleted copy constructor of 'foo'
foo h = f;
^ ~
move.cpp:4:3: note: copy constructor is implicitly deleted because 'foo' has a user-declared move constructor
foo(foo&&) = delete;
^
move.cpp:12:5: error: object of type 'foo' cannot be assigned because its copy assignment operator is implicitly deleted
f = std::move(f);
^
move.cpp:4:3: note: copy assignment operator is implicitly deleted because 'foo' has a user-declared move constructor
foo(foo&&) = delete;
^
move.cpp:13:5: error: object of type 'foo' cannot be assigned because its copy assignment operator is implicitly deleted
f = f;
^
move.cpp:4:3: note: copy assignment operator is implicitly deleted because 'foo' has a user-declared move constructor
foo(foo&&) = delete;
^
4 errors generated.<br></pre></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><br>
><br>
>><br>
>> + bool match(Instruction *I, const DataLayout *DL, bool isEQ) {<br>
>><br>
>> Especially given that “match" now conflicts with the match from PatternMatchers.h, is there a more descriptive name? I believe this does matching as well as comparison with an optional CompValue.<br>
><br>
> What about matchInstruction() ?<br>
><br>
<br>
</span>Sure, I have no desire to bikeshed :-)<br>
<span class=""><br>
> I updated the patch: <a href="http://reviews.llvm.org/D6333" target="_blank">http://reviews.llvm.org/D6333</a><br>
><br>
<br>
</span>LGTM<br>
<div class=""><div class="h5"><br>
> Mehdi<br>
><br>
><br>
>><br>
>> All in all, I think this switch to a struct and methods is an improvement over the many parameters and state that were there previously. LGTM.<br>
>><br>
>><br>
>>> On Nov 19, 2014, at 8:00 PM, Mehdi Amini <<a href="mailto:mehdi.amini@apple.com">mehdi.amini@apple.com</a>> wrote:<br>
>>><br>
>>> <0001-SimplifyCFG-Refactor-GatherConstantCompares-result-i.patch><br>
>><br>
><br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>