hawkinsw added inline comments.
================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:66
+ case 'c':
+ if (option_arg.getAsInteger(0, m_count) || (m_count < 0)) {
m_count = UINT32_MAX;
----------------
fixathon wrote:
> If getAsInteger() fails we trigger the error path.
> If getAsInteger() succeeds, but the fetched value (side-effect) is negative
> we trigger the error path.
>
> I'm not 100% happy with my own code here..
> getAsInteger() somewhat counter-intuitively returns *true* on failure, and
> there's reliance on the short-circuit evaluation sequence-points for correct
> evaluation when the 1st part is setting value via side-effect, and the second
> validates it. This makes the expression somewhat hard to reason about.
>
> Also mutating **m_count** directly may result in a non-atomic transition to
> its new valid state in case when getAsInteger() fetches negative input, which
> is less than perfect.
>
> That said, this code is just parsing command options, in a single-threaded
> manner, so that shouldn't matter.
I am not sure if this helps or not, but ...
`getAsInteger` is the exact interface that the LLVM command-line parsing
library uses for parsing numeric options. I looked through the codebase for
other places that do the same type of operation to see if they use a different
technique and could not find anything.
To address your non-atomic comment, we could introduce a temporary? Given your
description of how this code will be used (single-threaded), that seems like
overkill?
I am no expert and I hope that being part of the discussion is helpful.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D131983/new/
https://reviews.llvm.org/D131983
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
