[PATCH] D63085: Provide a fix-it hint for -Wswitch, which adds missing cases. If there are >3 cases, the inserted text will contain newlines so it will not be shown in console output (but will be appl

[Trass3r via Phabricator via cfe-commits]

Trass3r added a comment.

That's the general approach for clang-tidy use too, rely on clang-format for 
formatting the fixes.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63085/new/

https://reviews.llvm.org/D63085

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63085: Provide a fix-it hint for -Wswitch, which adds missing cases. If there are >3 cases, the inserted text will contain newlines so it will not be shown in console output (but will be appl

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D63085#3384302 , @iamsergio wrote:

> I don't think we need to worry about formatting, that's the IDE's job (and 
> whatever formatter it uses).
> In general, the code that knows how to warn should also be the one that knows 
> how to fix it, this looks like a good place to me.
>
> Can we unblock this ? Probably this anymore though.

I don't think formatting concerns should block progress on this; we expect 
users to run clang-format to fix formatting issues instead of hoping the fix-it 
system gets it right.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63085/new/

https://reviews.llvm.org/D63085

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63085: Provide a fix-it hint for -Wswitch, which adds missing cases. If there are >3 cases, the inserted text will contain newlines so it will not be shown in console output (but will be appl

[Sergio Martins via Phabricator via cfe-commits]

iamsergio added a comment.
Herald added a subscriber: usaxena95.
Herald added a project: All.

I don't think we need to worry about formatting, that's the IDE's job (and 
whatever formatter it uses).
In general, the code that knows how to warn should also be the one that knows 
how to fix it, this looks like a good place to me.

Can we unblock this ? Probably this anymore though.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63085/new/

https://reviews.llvm.org/D63085

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63085: Provide a fix-it hint for -Wswitch, which adds missing cases. If there are >3 cases, the inserted text will contain newlines so it will not be shown in console output (but will be appl

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

One major drawback that I see is the lack of indentation (and other format 
options) in the added code.
Should we have this fix at a higher level that can have formatting (either now 
or in the future)? E.g. in `clangd` directly?




Comment at: lib/Sema/SemaStmt.cpp:1208
+llvm::raw_string_ostream NewCasesOS(NewCases);
+// Include \n to separate new cases added if there are many.
+// This suppresses printing the (long) insert text.

Maybe always put each `case` on a new line?
From my experience, having multiple cases on the same line is not very common.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63085/new/

https://reviews.llvm.org/D63085



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63085: Provide a fix-it hint for -Wswitch, which adds missing cases. If there are >3 cases, the inserted text will contain newlines so it will not be shown in console output (but will be appl

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Ping... it'd be nice to have for clangd-9, though it's getting late.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63085/new/

https://reviews.llvm.org/D63085



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63085: Provide a fix-it hint for -Wswitch, which adds missing cases. If there are >3 cases, the inserted text will contain newlines so it will not be shown in console output (but will be appl

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay.
Herald added a project: clang.

Lifted the helper for spelling one scope from another from
SemaCodeComplete -> AST, to reuse it. It should be useful in other contexts too.


Repository:
  rC Clang

https://reviews.llvm.org/D63085

Files:
  include/clang/AST/DeclBase.h
  lib/AST/DeclBase.cpp
  lib/Sema/SemaCodeComplete.cpp
  lib/Sema/SemaLookup.cpp
  lib/Sema/SemaStmt.cpp
  test/FixIt/fixit-enum.cpp

Index: test/FixIt/fixit-enum.cpp
===
--- /dev/null
+++ test/FixIt/fixit-enum.cpp
@@ -0,0 +1,19 @@
+// RUN: cp %s %t.cpp
+// RUN: not %clang_cc1 -fixit %t.cpp -Werror
+// RUN: %clang_cc1 %t.cpp -Werror
+
+namespace ns {
+inline namespace q { namespace { enum { A, B, C }; } }
+} // namespace ns
+
+namespace ns2 {
+enum class F { X, Y, Z };
+
+void test(F f, decltype(ns::A) g) {
+  switch (f) {}
+  // CHECK: YYY
+
+  switch (g) {}
+  // CHECK: ZZZ
+}
+} // namespace ns2
Index: lib/Sema/SemaStmt.cpp
===
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -1193,14 +1193,31 @@
 
   // Produce a nice diagnostic if multiple values aren't handled.
   if (!UnhandledNames.empty()) {
-DiagnosticBuilder DB = Diag(CondExpr->getExprLoc(),
-TheDefaultStmt ? diag::warn_def_missing_case
-   : diag::warn_missing_case)
-   << (int)UnhandledNames.size();
+DiagnosticBuilder DB =
+Diag(CondExpr->getExprLoc(), TheDefaultStmt
+ ? diag::warn_def_missing_case
+ : diag::warn_missing_case)
+<< (int)UnhandledNames.size();
 
 for (size_t I = 0, E = std::min(UnhandledNames.size(), (size_t)3);
  I != E; ++I)
   DB << UnhandledNames[I];
+
+std::string NewCases;
+llvm::raw_string_ostream NewCasesOS(NewCases);
+// Include \n to separate new cases added if there are many.
+// This suppresses printing the (long) insert text.
+char Sep = UnhandledNames.size() > 3 ? '\n' : ' ';
+const NestedNameSpecifier *Qual =
+CurContext->getQualifierFor(ED->isScoped() ? ED : ED->getParent());
+for (const auto& Name : UnhandledNames) {
+  NewCasesOS << "case ";
+  if (Qual)
+Qual->print(NewCasesOS, Context.getPrintingPolicy());
+  NewCasesOS << Name << ':' << Sep;
+}
+NewCasesOS << "break;";
+DB << FixItHint::CreateInsertion(SS->getEndLoc(), NewCasesOS.str());
   }
 
   if (!hasCasesNotInSwitch)
Index: lib/Sema/SemaLookup.cpp
===
--- lib/Sema/SemaLookup.cpp
+++ lib/Sema/SemaLookup.cpp
@@ -20,6 +20,7 @@
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
+#include "clang/AST/NestedNameSpecifier.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Lex/HeaderSearch.h"
Index: lib/Sema/SemaCodeComplete.cpp
===
--- lib/Sema/SemaCodeComplete.cpp
+++ lib/Sema/SemaCodeComplete.cpp
@@ -648,50 +648,6 @@
   return iterator(DeclOrVector.get()->end());
 }
 
-/// Compute the qualification required to get from the current context
-/// (\p CurContext) to the target context (\p TargetContext).
-///
-/// \param Context the AST context in which the qualification will be used.
-///
-/// \param CurContext the context where an entity is being named, which is
-/// typically based on the current scope.
-///
-/// \param TargetContext the context in which the named entity actually
-/// resides.
-///
-/// \returns a nested name specifier that refers into the target context, or
-/// NULL if no qualification is needed.
-static NestedNameSpecifier *
-getRequiredQualification(ASTContext , const DeclContext *CurContext,
- const DeclContext *TargetContext) {
-  SmallVector TargetParents;
-
-  for (const DeclContext *CommonAncestor = TargetContext;
-   CommonAncestor && !CommonAncestor->Encloses(CurContext);
-   CommonAncestor = CommonAncestor->getLookupParent()) {
-if (CommonAncestor->isTransparentContext() ||
-CommonAncestor->isFunctionOrMethod())
-  continue;
-
-TargetParents.push_back(CommonAncestor);
-  }
-
-  NestedNameSpecifier *Result = nullptr;
-  while (!TargetParents.empty()) {
-const DeclContext *Parent = TargetParents.pop_back_val();
-
-if (const auto *Namespace = dyn_cast(Parent)) {
-  if (!Namespace->getIdentifier())
-continue;
-
-  Result = NestedNameSpecifier::Create(Context, Result, Namespace);
-} else if