zequanwu marked an inline comment as done.
zequanwu added inline comments.
================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:1518
UsesMap uses;
+ UsesMap constRefUses;
----------------
rsmith wrote:
> zequanwu wrote:
> > rnk wrote:
> > > If possible, it would be nice to avoid having a second map.
> > I use second map to let the new warning be orthogonal to existing warnings.
> > That means when we have both `-Wuninitialized` and
> > `-Wno-uninitialized-const-reference`, the old test cases about
> > uninitialized variables should all passed. Otherwise, I need to somehow
> > detect the presence of `-Wno-uninitialized-const-reference`, which I don't
> > see a way to do that. Consider this code if we have only one map:
> > ```
> > void consume_const_ref(const int &n);
> > int test_const_ref() {
> > int n;
> > consume_const_ref(n);
> > return n;
> > }
> > ```
> > No matter if the flag`-Wno-uninitialized-const-reference` is present or
> > not, we still need to add `n` in `consume_const_ref(n);` to `uses` map and
> > then let `S.Diag` to decide to diagnose it or not depend on the presence of
> > `-Wno-uninitialized-const-reference`. If the flag is given, `S.Diag` will
> > just ignore the warning, but uninit var warning will diagnose if
> > `Use.getKind()` returns `Always` in
> > https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/AnalysisBasedWarnings.cpp#L814
> > . That is not desired. Since the original behavior is to ignore the `n` in
> > `consume_const_ref(n);` by not adding it to the `uses` map.
> >
> > If there is a way to detect the presence of
> > `-Wno-uninitialized-const-reference` or to know if
> > `Sema::Diag(SourceLocation Loc, unsigned DiagID)` actually diagnosed a
> > warning, the use of second map can be avoid.
> Interesting testcase :)
>
> Ideally we want the set of diagnostics from a particular compilation to be
> the same you'd get by building with `-Weverything` and then filtering out the
> ones you didn't want. So it's better to not ask whether
> `-Wuninitialized-const-reference` is enabled or not.
>
> I think we only populate this map after we've found an uninitialized use; if
> so, the cost of a second map seems likely to be acceptable.
>
> FYI, there is a mechanism to ask the "is this diagnostic enabled?" question,
> but generally we'd prefer if it's only used to avoid doing work that would be
> redundant if the warning is disabled. You can use
> `Diag.isIgnored(diag::warn_..., Loc)` to determine if a diagnostic with the
> given ID would be suppressed at the given location. (In fact, it looks like
> this is already being called in a later change in this file.)
I think second map is necessary. So, `Diag.isIgnored` is not abused.
================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:1590-1600
+ // flush all const reference uses diags
+ for (const auto &P : constRefUses) {
+ const VarDecl *vd = P.first;
+ const MappedType &V = P.second;
+
+ UsesVec *vec = V.getPointer();
+ for (const auto &U : *vec) {
----------------
rsmith wrote:
> Do we want any idiomatic-self-init special-case handling here? For example:
>
> ```
> void f(const int&);
> void g() {
> int a = a;
> f(a);
> }
> ```
>
> Following the logic above, should that warn on the `int a = a;` not on the
> `f(a)` call? Or should we warn only on the `f(a)` call itself in this case?
> It seems like it might be surprising to say that `a` is "uninitialized" here,
> since an initializer was provided, even though it was a garbage one.
For this case, I think we should warn at `int a = a`, like the comment in
`DiagnoseUninitializedUse` said,
https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/AnalysisBasedWarnings.cpp#L986-L996
`f(a)` is considered as accessing `a`.
================
Comment at: clang/test/SemaCXX/warn-uninitialized-const-reference.cpp:23
+ int k = const_use(k); // expected-warning {{variable 'k' is uninitialized
when used within its own initialization}}
+ A a2 = const_use_A(a2); // expected-warning {{variable 'a2' is
uninitialized when used within its own initialization}}
+ A a3(const_ref_use_A(a3)); // expected-warning {{variable 'a3' is
uninitialized when passes as a const reference parameter here}}
----------------
aeubanks wrote:
> For my knowledge, is this to make sure that this other warning takes
> precedence over the one introduced in this change? If it is, a comment would
> be nice.
No, original behavior of `-Wuninitialized` for line 24 is silence. Now the new
warning will be emitted because it is const use of uninitialized variable.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79895/new/
https://reviews.llvm.org/D79895
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
