Eliminates uninitialized warning for volatile variables.
Thanks! Can you add a test somewhere? grep -R W.*uninitialized test/Sema* will probably show you the existing test for this code. (Another technique: Comment some of the existing tests, run ninja check-clang, and see which tests start failing.)
You can run an individual test via bin/llvm-lit ../../llvm/tools/clang/test/Sema/mytest.cc.
Should the check be in DiagUninitUse()? Or is there a reason this one should happen for volatiles?
Thanks for the test!
What about the other cases in this switch? Does a volatile still warn in those cases? Should it? (Probably not?)
Right now the approach is to add an early return when the warning is emitted. Maybe this change should instead be somewhere where we compute if a var could be uninitialized, and that should always be false for volatile variables? Then future other warnings looking at that bit would get volatiles right for free.
Is it possible to test that both branches have this early return now?
Thanks, this is looking pretty good! From clicking around a bit on cs, do you think it's better to put the check where you have it, or is maybe http://llvm-cs.pcc.me.uk/tools/clang/lib/Analysis/UninitializedValues.cpp#36 more appropriate? I think having it where you have it means saying "we can reason about volatile vars, and we want to treat them as initialized" while the other spot means "we can't reason about volatile variables at all". I don't know which place is better (of someone else reading this knows, please speak up). If the other place makes your test fail, having the check where you have it is probably fine. Else I'd say that moving the check up is probably better.
|763 ↗||(On Diff #84026)|
No idea what should happen with volatile int x = x -- but I've never seen that in practice, so it probably doesn't matter too much either way.
What's the motivation for this? The placement of a local volatile variable is still under the compiler's direction, and unless the address escapes, we still assume we can reason about its aliasing (and, thus, whether or not it is initialized).
Please fix the spelling errors in the titel / summary before commit.
I somewhat agree with Hal -- I think this is too aggressive. Common use cases for local volatile include atomic ops or returns-twice functions like setjmp/longjmp.
Disabling the warning in those cases has a high chance of hiding real problems. I would find it much more useful to move this case into a warning subgroup, so that
it can be selectively disabled by command line or pragma.