clayborg accepted this revision.
clayborg added inline comments.
This revision is now accepted and ready to land.
================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:66
+ case 'c':
+ if (option_arg.getAsInteger(0, m_count) || (m_count < 0)) {
m_count = UINT32_MAX;
----------------
clayborg wrote:
> hawkinsw wrote:
> > 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.
> Can we rely on ordering here? Or do we need to have two if statements, first
> one for getAsInteger and a second nested on for "m_count < 0"? I would be
> safe if we don't know the right answer
Actually this should work just fine. Otherwise things like "if (ptr &&
ptr->...)" wouldn't work.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D131983/new/
https://reviews.llvm.org/D131983
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
