jrtc27 added a comment.

In D137517#4045315 <https://reviews.llvm.org/D137517#4045315>, @fpetrogalli 
wrote:

> In D137517#4045299 <https://reviews.llvm.org/D137517#4045299>, @jrtc27 wrote:
>
>> In D137517#4042875 <https://reviews.llvm.org/D137517#4042875>, @fpetrogalli 
>> wrote:
>>
>>> In D137517#4042758 <https://reviews.llvm.org/D137517#4042758>, @fpetrogalli 
>>> wrote:
>>>
>>>> After submitting this, I had to revert it 
>>>> <https://github.com/llvm/llvm-project/commit/8bd65e535fb33bc48805bafed8217b16a853e158>
>>>>  because of failures like 
>>>> https://lab.llvm.org/buildbot/#/builders/225/builds/12367/steps/5/logs/stdio
>>>
>>> I have resubmitted with what I hope is the right fix (I could not reproduce 
>>> any of the failures I was seeing in buildbot, on my machine the build is 
>>> fine).
>>>
>>> The new commit is at 
>>> https://github.com/llvm/llvm-project/commit/ac1ffd3caca12c254e0b8c847aa8ce8e51b6cfbf
>>>  - in the commit message I have explained what I have changed WRT this 
>>> original patch. I have added  the
>>> tablegen target `RISCVTargetParserTableGen` in the `DEPENDS` list of 
>>> `clangDriver` and `clangBasic`. This makes sure that the `.*inc` file with 
>>> theist o the CPU is available even if `LLVMTargetParser` has not been built 
>>> yet.
>>
>> But you didn't use the proper Differential Revision tag, so the diff here 
>> didn't get updated to reflect the amended version committed :(
>
> What should I have done? Add  the `Differential Revision: 
> https://reviews.llvm.org/D137517` as the last line of the commit with the 
> rework? I wasn't aware of this for reworks.

Yes, it should have the same trailer as the original commit, otherwise it won't 
be correctly tracked by Phabricator. A "rework" isn't special, it's revert, 
reopen the revision, update the revision and land the revision again. If 
re-review isn't needed then you can skip some of the middle, but that's it. 
Though in this case I do think re-review was warranted, the new clang 
dependency seems a bit dubious and hints at the design not being quite right.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137517/new/

https://reviews.llvm.org/D137517

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to