Re: Tests added to the Templight pull request

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: Tests added to the Templight pull request

Eric Fiselier via cfe-dev
Hey Abel, is there any chance you can add the patches to https://reviews.llvm.org/D5767 instead of starting a new review? Given the context on the other patch is rather extensive, that would help review :)

On Wed, Oct 11, 2017 at 4:05 PM Manuel Klimek <[hidden email]> wrote:
Hi, thanks for letting me know, excited to see this make progress \o/

Generally, if you follow the guidelines here: https://llvm.org/docs/Phabricator.html
you'll get a code review that is cc'ed to the cfe-commits list (you can also put me in as reviewer), which is generally how people find patches.

You can also continue the old review by manually specifying it when uploading the patch via arc or in the UI.


On Wed, Oct 11, 2017 at 1:16 PM Abel Sinkovics <[hidden email]> wrote:
Hi Manuel,

Kristóf (cc'd) has implemented the missing tests for the Templight pull
request for Clang. I have updated the patch, but the system seems to
have created another pull request as the result of this. You can find it
here: https://reviews.llvm.org/D38818

Please let us know if there are further things we should address.

Thanks,
   Ábel



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Tests added to the Templight pull request

Eric Fiselier via cfe-dev

Hi Manuel,

I've tried to do so, but the UI doesn't let me do that. I can choose between creating a new revision and updating the one I recently created (D38818). I assume this is because D5767 was created by Mikael (cc'd), not me.

Regards,
  Ábel


Am 2017-10-16 um 00:06 schrieb Manuel Klimek:
Hey Abel, is there any chance you can add the patches to https://reviews.llvm.org/D5767 instead of starting a new review? Given the context on the other patch is rather extensive, that would help review :)

On Wed, Oct 11, 2017 at 4:05 PM Manuel Klimek <[hidden email]> wrote:
Hi, thanks for letting me know, excited to see this make progress \o/

Generally, if you follow the guidelines here: https://llvm.org/docs/Phabricator.html
you'll get a code review that is cc'ed to the cfe-commits list (you can also put me in as reviewer), which is generally how people find patches.

You can also continue the old review by manually specifying it when uploading the patch via arc or in the UI.


On Wed, Oct 11, 2017 at 1:16 PM Abel Sinkovics <[hidden email]> wrote:
Hi Manuel,

Kristóf (cc'd) has implemented the missing tests for the Templight pull
request for Clang. I have updated the patch, but the system seems to
have created another pull request as the result of this. You can find it
here: https://reviews.llvm.org/D38818

Please let us know if there are further things we should address.

Thanks,
   Ábel




_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Tests added to the Templight pull request

Eric Fiselier via cfe-dev
You need to "commandeer" a revision before you can add diff to it. (That option is in the action menu above the main comment box when viewing it).

On Mon, Oct 16, 2017 at 4:04 PM, Abel Sinkovics via cfe-dev <[hidden email]> wrote:

Hi Manuel,

I've tried to do so, but the UI doesn't let me do that. I can choose between creating a new revision and updating the one I recently created (D38818). I assume this is because D5767 was created by Mikael (cc'd), not me.

Regards,
  Ábel


Am 2017-10-16 um 00:06 schrieb Manuel Klimek:
Hey Abel, is there any chance you can add the patches to https://reviews.llvm.org/D5767 instead of starting a new review? Given the context on the other patch is rather extensive, that would help review :)

On Wed, Oct 11, 2017 at 4:05 PM Manuel Klimek <[hidden email]> wrote:
Hi, thanks for letting me know, excited to see this make progress \o/

Generally, if you follow the guidelines here: https://llvm.org/docs/Phabricator.html
you'll get a code review that is cc'ed to the cfe-commits list (you can also put me in as reviewer), which is generally how people find patches.

You can also continue the old review by manually specifying it when uploading the patch via arc or in the UI.


On Wed, Oct 11, 2017 at 1:16 PM Abel Sinkovics <[hidden email]> wrote:
Hi Manuel,

Kristóf (cc'd) has implemented the missing tests for the Templight pull
request for Clang. I have updated the patch, but the system seems to
have created another pull request as the result of this. You can find it
here: https://reviews.llvm.org/D38818

Please let us know if there are further things we should address.

Thanks,
   Ábel




_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Tests added to the Templight pull request

Eric Fiselier via cfe-dev

Thank you James, I could update the original (D5767) review.

Regards,
  Ábel


Am 2017-10-17 um 19:19 schrieb James Y Knight:
You need to "commandeer" a revision before you can add diff to it. (That option is in the action menu above the main comment box when viewing it).

On Mon, Oct 16, 2017 at 4:04 PM, Abel Sinkovics via cfe-dev <[hidden email]> wrote:

Hi Manuel,

I've tried to do so, but the UI doesn't let me do that. I can choose between creating a new revision and updating the one I recently created (D38818). I assume this is because D5767 was created by Mikael (cc'd), not me.

Regards,
  Ábel


Am 2017-10-16 um 00:06 schrieb Manuel Klimek:
Hey Abel, is there any chance you can add the patches to https://reviews.llvm.org/D5767 instead of starting a new review? Given the context on the other patch is rather extensive, that would help review :)

On Wed, Oct 11, 2017 at 4:05 PM Manuel Klimek <[hidden email]> wrote:
Hi, thanks for letting me know, excited to see this make progress \o/

Generally, if you follow the guidelines here: https://llvm.org/docs/Phabricator.html
you'll get a code review that is cc'ed to the cfe-commits list (you can also put me in as reviewer), which is generally how people find patches.

You can also continue the old review by manually specifying it when uploading the patch via arc or in the UI.


On Wed, Oct 11, 2017 at 1:16 PM Abel Sinkovics <[hidden email]> wrote:
Hi Manuel,

Kristóf (cc'd) has implemented the missing tests for the Templight pull
request for Clang. I have updated the patch, but the system seems to
have created another pull request as the result of this. You can find it
here: https://reviews.llvm.org/D38818

Please let us know if there are further things we should address.

Thanks,
   Ábel




_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev




_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev