[PATCH] D121159: llvm-nm should flush and error check output

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

In D121159#3366232 <https://reviews.llvm.org/D121159#3366232>, @jhenderson wrote:

> In D121159#3366227 <https://reviews.llvm.org/D121159#3366227>, @davide wrote:
>
>> In D121159#3366196 <https://reviews.llvm.org/D121159#3366196>, @jhenderson wrote:
>>
>>> The pre-merge check is  failing to build this patch.
>>>
>>> There are loads of LLVM tools that print to stdout. This "fixes" just one of them. If there is an issue, it's in all the tools, so needs patching somewhere else. I'm also not convinced it should be needed:
>>
>> I think this is for UNIX03 compliance. Not all the tools have to be compliant.
>
> Perhaps not, but there's presumably a good reason for compliance requirements. Anyway, I'd be surprised if llvm-nm is any more special than e.g. llvm-readelf or llvm-objdump in this area.
>
>>> 2. stdout is flushed on exit automatically: https://stackoverflow.com/questions/15911517/is-there-a-guarantee-of-stdout-auto-flush-before-exit-how-does-it-work
>>
>> If stdout is flushed before exit, that's fine. What about `stderr` ?
>
> `stderr` is unbuffered. It shouldn't need flushing.
>
>>> How does explicit flushing help us with output errors. More importantly, why do we care?
>>
>> We care for POSIX compliance. Apple can keep this patch downstream, but I don't see the harm of having this one here.
>
> Harm comes from having code that is unnecessary, since it does nothing that doesn't automatically happen. Future developers will come along and see it and delete it because it appears to be pointless. You didn't answer the first half of that comment though: how does explicit flushing help us? The patch description talks about error checking a stream, but this modification doesn't seem to do anything of the sort.

Fair. I'll come up with a testcase where this matters (taken from the suite) and propose a new patch. For now, I'm withdrawing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121159