Snape3058 wrote: > > > That said, as 'UB', self-assign's 'behavior' through LLVM-IR is > > > effectively no effect, even if it is wasted instructions (that is, `mov > > > eax, eax` won't change anything at all except perhaps invalidating > > > speculation/etc). > > > IMO the clang behavior is completely making sense: we diagnose when this > > > COULD be problematic (such as causing copy constructor from a > > > not-yet-life-constructed self). > > > I'm a bit concerned about: `The self-assignment initialization Type var = > > > var; is an idiom in C code.` from the commit message. Is this true? Is > > > this REALLY that common? To what end? > > > > I've seen it in C code but don't have a good idea of prevalence. The uses > > I've seen were for silencing "unused variable" warnings in C89 code but I'm > > not certain how crucial they are to support. > > Clang and GCC have some difference in behavior for how we handle > > `-Winit-self` and `-Wuninitialized`, too. It's not clear whether those > > differences are intentional or not. > > My understanding is that Clang's warnings is at least reasonable, so I'd > probably need to see some justification to change that? Carving out this > exception in the static-analyzer is a 'neutral' for me, though I've done SOME > googling and see some suggestions to do self-init to suppress > `-Wuninitialized` on google. The carve-out of `-Winit-self` being separate > seems to be supporting evidence for this... so I guess I'm > slightly-above-neutral?
At least we are not completely removing this check for self-assigned variables. When they remain uninitialized until their first access, we will still report this issue for them. Besides, this patch only suppresses the self-assignment in the format of `Type var = var;`, other self-assignments, such as self-assignments in an assignment expression, are not involved. For the `-Winit-self` option, I do not know whether it is a good idea for the static analyzer to follow a frontend diagnostic option. IMO, perhaps not. For the commit message, I don't know exactly whether it is an "idiom" that has a name and is widely used in the community. But the person who raised this issue said so, and it is really used in the wild. If necessary, we can update this before committing. @AaronBallman @erichkeane Are there still any things blocking merging this patch? You can still discuss this topic after this patch is merged. @steakhal @NagyDonat What do you think about the new version with `-Wno-uninitialized` added, as well as the discussions above? https://github.com/llvm/llvm-project/pull/187530 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
