[clang] 24ab9b5 - Generalize the pass registration mechanism used by Polly to any third-party tool

[Eric Christopher via cfe-commits]

On Wed, Jan 8, 2020 at 3:07 PM Michael Kruse <llvm-commits at meinersbur.de>
wrote:

> Am Di., 7. Jan. 2020 um 18:57 Uhr schrieb Eric Christopher via
> cfe-commits <cfe-commits at lists.llvm.org>:
> >> Is there anything I should have done? Probably reaching llvm-dev before
> >> commiting. Reaching the right reviewers has always been a challenge to
> me, I had
> >> hoped that the mail to llvm-dev would trigger some subscription :-)
> >
> >
> > Hrm. Probably finding some different reviewers, but I can't fault your
> attempts here. Usually you can look at the last few people to make
> substantial work in an area and loop them in via git log. :)
>
> In my experience adding more reviewers usually rarely to more people
> looking into the patch. Especially the people that make substantial
> work in an area and/or are code owners are added as reviewers to very
> many commits and cannot look into all of them.
>

As one of those added to a lot of patches I have a lot of sympathy here.
The answer isn't to avoid adding people with expertise in an area and no
one on the approval list had expertise or had recently hacked on the code.


> Note that in the patch in question, there are comments from people not
> in the reviewer list. For example, Mehdi with whom I also talked about
> this patch at the last LLVM DevMtg.
>
>
Yes, but none of them ack'd it.


> We also have a policy that allows post-commit reviews and reverts.
> Therefore I think we should not put that bar for allowing to commit
> too high.
>
>
I could, of course, have just asked this to be reverted I guess.


>
> >> >  ; CHECK-EP-VECTORIZER-START-NEXT: Running pass: NoOpFunctionPass
> >> > +; CHECK-EXT: Running pass: {{.*}}::Bye on foo
> >> >
> >> > Why is this running on every test of the pass manager? It should be
> an example
> >> > run in the examples directory and not on by default? Same for every
> other PM
> >> > test. This seems like a bug?
> >>
> >> It's not. When the examples are active and if the appropriate cmake
> flag is set
> >> (which is not the case by default), the pass is linked in statically,
> and is run
> >> in the default pipeline. The CHECK-EXT prefix is disabled otherwise.
> That's one
> >> of the configuration I did test :-)
> >>
> >
> > I really don't think this is ideal. The examples directory shouldn't
> affect tests being run or not or in what way. Can we back this part out and
> talk about it a bit more? I don't think we should need to do this to test
> the functionality.
>
> In a previous discussion [1] we discussed adding tutorial code to
> repository which naturally should also be tested to avoid bit-rot. How
> do you suggest to testing of examples should work? Note that LLVMHello
> is also tested, but not in the example directory.
>

llvm/tests/examples?

Thanks.

-eric
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200108/9a672330/attachment.html>