Am Di., 7. Jan. 2020 um 18:57 Uhr schrieb Eric Christopher via
cfe-commits <cfe-commits@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.
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.
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.
>> > ; 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.
For this patch I suggested in the review to add another flag to the
Bye example and only if set the the Bye pass is added to the pass
manager (but in a different context: to avoid shell expansion hack
that would not run on Windows anyway). This would stop the pass
pipeline pass to fail because of a new pass in there and can be tested
separately. At the end I had no preference of how it should be tested.
[1] http://lists.llvm.org/pipermail/llvm-dev/2019-October/136159.html
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
