[PATCH] D46061: [DEBUGINFO, NVPTX] Disable emission of ',debug' option if only debug directives are allowed.

[David Blaikie via llvm-commits]

Generally I think it's best to get some closure from the person who asked
the questions/raised the issues to make sure they've understood/agreed with
the solutions. Just a thought/suggestion. I wouldn't bother reverting this
now (Eric can ask for that if he feels his concerns weren't addressed &
they're significant enough to warrant a rollback while further discussion
occurs) - but would generally suggest not approving or committing a patch
if it has outstanding (not confirmed as addressed by the original reviewer)
comments like this. Usually such approval should be in the form "This looks
good to me, but make sure <other reviewer> has confirmed that their
concerns are addressed" - that makes it easy for "other reviewer" too,
because it means they don't have to worry about taking on the entire burden
of reviewing the patch, so they know they can just come back and check the
specific concerns they expressed were addressed to their
satisfaction/understanding. (means sometimes a quick pestering on IRC, etc,
might help encourage them to spend the relatively short amount of time to
review the specific points they'd raised previously)

On Fri, Nov 2, 2018 at 7:52 AM Alexey Bataev <a.bataev at outlook.com> wrote:

> No, I reworked the patch according to his advice, so I assume everything is fine here
> -------------
> Best regards,
> Alexey Bataev
>
> 02.11.2018 10:50, David Blaikie пишет:
>
> Looks like Eric still had some questions here? Or at least hadn't
> commented about whether the answers/changes addressed his concerns?
>
> On Thu, Nov 1, 2018 at 12:45 PM Paul Robinson via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> probinson accepted this revision.
>> probinson added a comment.
>> This revision is now accepted and ready to land.
>>
>> LGTM
>>
>>
>>
>> ================
>> Comment at: test/DebugInfo/NVPTX/debug-file-loc-only.ll:3
>> +
>> +; // Bitcode int this test case is reduced version of compiled code
>> below:
>> +;extern "C" {
>> ----------------
>> Typo: "Bitcode in this test..." (not "int").
>>
>>
>>
>> Repository:
>>   rL LLVM
>>
>> https://reviews.llvm.org/D46061
>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181102/e97e0fdb/attachment.html>