In D125349#3509073, @aaron.ballman wrote:

It's interesting to note that an_atomic_uint = an_atomic_uint + an_enum_value works correctly: https://godbolt.org/z/cvP9e6nh7. I was trying to figure out whether the atomic qualifier is properly stripped for the compound operator. When I run under a debugger and dump the AST for Args[0], I get: DeclRefExpr 0x26efcf87f88 '_Atomic(unsigned int)' lvalue Var 0x26efcf44cb8 'an_atomic_uint' '_Atomic(unsigned int)' which seems like it may be the root cause of the problem here (tough to say given that this is a C extension in C++ though). The lvalue conversion that takes place for an_atomic_uint should drop the atomic qualifier per C2x 6.3.2.1p2.

In D125349#3509546, @ahatanak wrote:

Is it not possible to handle this similarly to volatile unsigned? If I replace _Atomic unsigned with volatile unsigned, I see LookupOverloadedBinOp succeed without having to strip volatile because addAssignmentArithmeticOverloads adds candidates with volatile types.

Reverting for now. Will take a look.

Ping.

Friendly ping.

Made the tests consistent with other pre-existing tests.

Sure, will do! I intended to keep the tests simple but understand the desire for consistency.

Thanks @jansvoboda11!

I noticed ReadVBR64 method below does the same arithmetics and keeping the implementation in sync seems like a no-brainer.

switched to assert
added the assert also to ReadVBR64

@nikic Sorry, my statement was incorrect. What I meant is that the method doesn't prevent UB via assert or runtime check of NumBits value.
I'm happy to go with assert if that'd be a better fit.

clang-format

rebased + improved error messages

I verified locally that reverting this patch fixes the build.
Reverted in fad7e491a0770ac4336934030ac67d77e7af5520 to unblock Green Dragon, etc.
@aaron.ballman Please take a look when you get a chance.

@aaron.ballman I believe this change broke the build starting with:

LGTM

LGTM if we have test coverage for all the cases.

Hi! Thank you for the clean-up :)

I will send an email to llvm-dev@ and add a release note once I finish that.

I just realized that for -fsanitize-recover the logic is inverted - it's actually -fno-sanitize-recover that's an explicit change from the default.
I will probably need to do the same thing I do for traps.

improved tests

Adding the comment would be great.

Thanks for working on this!

Friendly ping

In D88332#2298301, @baloghadamsoftware wrote:

I would even go further. Why Clang Static Analyzer? I see nothing path-sensitive here. IMHO a better place to implement this functionality is Clang-Tidy.

In D88332#2297374, @NoQ wrote:

Hmm, if we could add an interface to CallDescriptionMap to accept AnyCall instead of CallEvent then we could use it for path-insensitive checkers as well. That'd have saved some lines of code.

Sounds interesting but I lack context to imagine the details. Would the map be still initialized by some ASTVisitor? Do you think there'd be any noticeable performance hit?

Had to fix the order of expected warnings in warning-wall.c
6fd8c69049a8 [clang] Update warning-wall.c test

Friendly ping.
I'd like to land this sooner than later - I agree it'd be great to have a regression test but my (possibly biased) opinion is that the change is also simple and obvious enough that we could land it without a test.

Friendly ping

Friendly ping

Created this for eventual post-commit review.

I made a typo in the commit message reference so this revision didn't get closed automatically.

Thank you Artem!

Thank you both for all the hints!

Simply check memory regions of LHS and RHS in assignments instead of AST pattern matching.

Nits were addressed (hopefully).

Attempt to write a unit test - so far can't reproduce even with unpatched master :(

Added test

Friendly ping

Hi @usaxena95 and @sammccall,

• renamed
• added tests

Sorry to bother you @NoQ - it'd be awesome if you could take a look!