[PATCH] SimplifyCFG: turn recursive GatherConstantCompares into iterative

[David Blaikie]

On Wed, Nov 19, 2014 at 10:28 PM, Michael Ilseman <milseman at apple.com>
wrote:

>
> > On Nov 19, 2014, at 10:03 PM, Mehdi Amini <mehdi.amini at apple.com> wrote:
> >
> >
> >> On Nov 19, 2014, at 8:42 PM, Michael Ilseman <milseman at apple.com>
> wrote:
> >>
> >> The git-diff got a little convoluted in it’s +'s and -'s, so I’ll trust
> that you kept the functionality the same.
> >>
> >> +  /// Don't want to copy this
> >> +  ConstantComparesGatherer(const ConstantComparesGatherer &) = delete;
> >> +
> >>
> >> If that’s the case, you also probably want to delete the assignment
> operator.
> >
> > It is not that important, it was just to avoid any mistake, but it is a
> private class. I removed it.
>
> 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.
>

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.

$ 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 at 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.



>
> >
> >>
> >> +  bool match(Instruction *I, const DataLayout *DL, bool isEQ) {
> >>
> >> 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.
> >
> > What about matchInstruction() ?
> >
>
> Sure, I have no desire to bikeshed :-)
>
> > I updated the patch: http://reviews.llvm.org/D6333
> >
>
> LGTM
>
> > Mehdi
> >
> >
> >>
> >> 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.
> >>
> >>
> >>> On Nov 19, 2014, at 8:00 PM, Mehdi Amini <mehdi.amini at apple.com>
> wrote:
> >>>
> >>> <0001-SimplifyCFG-Refactor-GatherConstantCompares-result-i.patch>
> >>
> >
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141120/914529be/attachment.html>