[PATCH] D48266: [Driver] Add -fno-digraphs

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes marked an inline comment as done.
jtbandes added a comment.

Ping again 


Repository:
  rL LLVM

https://reviews.llvm.org/D48266



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

[PATCH] D48266: [Driver] Add -fno-digraphs

[Jacob Bandes-Storch via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL337232: [Driver] Add -fno-digraphs (authored by jtbandes, 
committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D48266?vs=155116=155809#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D48266

Files:
  cfe/trunk/include/clang/Driver/Options.td
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/test/Lexer/digraph.c


Index: cfe/trunk/include/clang/Driver/Options.td
===
--- cfe/trunk/include/clang/Driver/Options.td
+++ cfe/trunk/include/clang/Driver/Options.td
@@ -1335,6 +1335,10 @@
 def fno_diagnostics_show_option : Flag<["-"], "fno-diagnostics-show-option">, 
Group;
 def fno_diagnostics_show_note_include_stack : Flag<["-"], 
"fno-diagnostics-show-note-include-stack">,
 Flags<[CC1Option]>, Group;
+def fdigraphs : Flag<["-"], "fdigraphs">, Group, Flags<[CC1Option]>,
+  HelpText<"Enable alternative token representations '<:', ':>', '<%', '%>', 
'%:', '%:%:' (default)">;
+def fno_digraphs : Flag<["-"], "fno-digraphs">, Group, 
Flags<[CC1Option]>,
+  HelpText<"Disallow alternative token representations '<:', ':>', '<%', '%>', 
'%:', '%:%:'">;
 def fno_declspec : Flag<["-"], "fno-declspec">, Group,
   HelpText<"Disallow __declspec as a keyword">, Flags<[CC1Option]>;
 def fno_dollars_in_identifiers : Flag<["-"], "fno-dollars-in-identifiers">, 
Group,
Index: cfe/trunk/test/Lexer/digraph.c
===
--- cfe/trunk/test/Lexer/digraph.c
+++ cfe/trunk/test/Lexer/digraph.c
@@ -1,6 +1,13 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -ffreestanding %s
-// expected-no-diagnostics
+// RUN: %clang_cc1 -DDIGRAPHS=1 -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -DDIGRAPHS=1 -fno-digraphs -fdigraphs -fsyntax-only -verify 
-ffreestanding %s
+// RUN: %clang_cc1 -fno-digraphs -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -fdigraphs -fno-digraphs -fsyntax-only -verify 
-ffreestanding %s
+// RUN: %clang_cc1 -std=c89 -DDIGRAPHS=1 -fdigraphs -fsyntax-only -verify 
-ffreestanding %s
+// RUN: %clang_cc1 -std=c89 -fno-digraphs -fsyntax-only -verify -ffreestanding 
%s
+
+#if DIGRAPHS
 
+// expected-no-diagnostics
 %:include 
 
 %:ifndef BUFSIZE
@@ -14,3 +21,15 @@
 d<:len:> = s<:len:>;
 %>
 %>
+#else
+
+// expected-error@+1 {{expected identifier or '('}}
+%:include 
+;
+// expected-error@+1 {{expected ')'}} expected-note@+1{{to match this '('}}
+void copy(char d<::>);
+
+// expected-error@+1 {{expected function body}}
+void copy() <% %>
+
+#endif
Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -3925,6 +3925,7 @@
   // Forward -f (flag) options which we can pass directly.
   Args.AddLastArg(CmdArgs, options::OPT_femit_all_decls);
   Args.AddLastArg(CmdArgs, options::OPT_fheinous_gnu_extensions);
+  Args.AddLastArg(CmdArgs, options::OPT_fdigraphs, options::OPT_fno_digraphs);
   Args.AddLastArg(CmdArgs, options::OPT_fno_operator_names);
   Args.AddLastArg(CmdArgs, options::OPT_femulated_tls,
   options::OPT_fno_emulated_tls);
Index: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
===
--- cfe/trunk/lib/Frontend/CompilerInvocation.cpp
+++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp
@@ -2173,6 +2173,8 @@
   Opts.GNUKeywords = Args.hasFlag(OPT_fgnu_keywords, OPT_fno_gnu_keywords,
   Opts.GNUKeywords);
 
+  Opts.Digraphs = Args.hasFlag(OPT_fdigraphs, OPT_fno_digraphs, Opts.Digraphs);
+
   if (Args.hasArg(OPT_fno_operator_names))
 Opts.CXXOperatorNames = 0;
 


Index: cfe/trunk/include/clang/Driver/Options.td
===
--- cfe/trunk/include/clang/Driver/Options.td
+++ cfe/trunk/include/clang/Driver/Options.td
@@ -1335,6 +1335,10 @@
 def fno_diagnostics_show_option : Flag<["-"], "fno-diagnostics-show-option">, Group;
 def fno_diagnostics_show_note_include_stack : Flag<["-"], "fno-diagnostics-show-note-include-stack">,
 Flags<[CC1Option]>, Group;
+def fdigraphs : Flag<["-"], "fdigraphs">, Group, Flags<[CC1Option]>,
+  HelpText<"Enable alternative token representations '<:', ':>', '<%', '%>', '%:', '%:%:' (default)">;
+def fno_digraphs : Flag<["-"], "fno-digraphs">, Group, Flags<[CC1Option]>,
+  HelpText<"Disallow alternative token representations '<:', ':>', '<%', '%>', '%:', '%:%:'">;
 def fno_declspec : Flag<["-"], "fno-declspec">, Group,
   HelpText<"Disallow __declspec as a keyword">, Flags<[CC1Option]>;
 def fno_dollars_in_identifiers : Flag<["-"], "fno-dollars-in-identifiers">, Group,
Index: cfe/trunk/test/Lexer/digraph.c

[PATCH] D48266: [Driver] Add -fno-digraphs

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added a comment.

Ping again 


Repository:
  rC Clang

https://reviews.llvm.org/D48266



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

[PATCH] D48266: [Driver] Add -fno-digraphs

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes updated this revision to Diff 155116.
jtbandes marked an inline comment as done.
jtbandes added a comment.

- include %:%: in help text


Repository:
  rC Clang

https://reviews.llvm.org/D48266

Files:
  include/clang/Driver/Options.td
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/Lexer/digraph.c


Index: test/Lexer/digraph.c
===
--- test/Lexer/digraph.c
+++ test/Lexer/digraph.c
@@ -1,6 +1,13 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -ffreestanding %s
-// expected-no-diagnostics
+// RUN: %clang_cc1 -DDIGRAPHS=1 -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -DDIGRAPHS=1 -fno-digraphs -fdigraphs -fsyntax-only -verify 
-ffreestanding %s
+// RUN: %clang_cc1 -fno-digraphs -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -fdigraphs -fno-digraphs -fsyntax-only -verify 
-ffreestanding %s
+// RUN: %clang_cc1 -std=c89 -DDIGRAPHS=1 -fdigraphs -fsyntax-only -verify 
-ffreestanding %s
+// RUN: %clang_cc1 -std=c89 -fno-digraphs -fsyntax-only -verify -ffreestanding 
%s
+
+#if DIGRAPHS
 
+// expected-no-diagnostics
 %:include 
 
 %:ifndef BUFSIZE
@@ -14,3 +21,15 @@
 d<:len:> = s<:len:>;
 %>
 %>
+#else
+
+// expected-error@+1 {{expected identifier or '('}}
+%:include 
+;
+// expected-error@+1 {{expected ')'}} expected-note@+1{{to match this '('}}
+void copy(char d<::>);
+
+// expected-error@+1 {{expected function body}}
+void copy() <% %>
+
+#endif
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2174,6 +2174,9 @@
   Opts.GNUKeywords = Args.hasFlag(OPT_fgnu_keywords, OPT_fno_gnu_keywords,
   Opts.GNUKeywords);
 
+  if (const Arg *A = Args.getLastArg(OPT_fdigraphs, OPT_fno_digraphs))
+Opts.Digraphs = A->getOption().matches(OPT_fdigraphs) ? 1 : 0;
+
   if (Args.hasArg(OPT_fno_operator_names))
 Opts.CXXOperatorNames = 0;
 
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3970,6 +3970,7 @@
   // Forward -f (flag) options which we can pass directly.
   Args.AddLastArg(CmdArgs, options::OPT_femit_all_decls);
   Args.AddLastArg(CmdArgs, options::OPT_fheinous_gnu_extensions);
+  Args.AddLastArg(CmdArgs, options::OPT_fdigraphs, options::OPT_fno_digraphs);
   Args.AddLastArg(CmdArgs, options::OPT_fno_operator_names);
   Args.AddLastArg(CmdArgs, options::OPT_femulated_tls,
   options::OPT_fno_emulated_tls);
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1334,6 +1334,10 @@
 def fno_diagnostics_show_option : Flag<["-"], "fno-diagnostics-show-option">, 
Group;
 def fno_diagnostics_show_note_include_stack : Flag<["-"], 
"fno-diagnostics-show-note-include-stack">,
 Flags<[CC1Option]>, Group;
+def fdigraphs : Flag<["-"], "fdigraphs">, Group, Flags<[CC1Option]>,
+  HelpText<"Enable alternative token representations '<:', ':>', '<%', '%>', 
'%:', '%:%:' (default)">;
+def fno_digraphs : Flag<["-"], "fno-digraphs">, Group, 
Flags<[CC1Option]>,
+  HelpText<"Disallow alternative token representations '<:', ':>', '<%', '%>', 
'%:', '%:%:'">;
 def fno_declspec : Flag<["-"], "fno-declspec">, Group,
   HelpText<"Disallow __declspec as a keyword">, Flags<[CC1Option]>;
 def fno_dollars_in_identifiers : Flag<["-"], "fno-dollars-in-identifiers">, 
Group,


Index: test/Lexer/digraph.c
===
--- test/Lexer/digraph.c
+++ test/Lexer/digraph.c
@@ -1,6 +1,13 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -ffreestanding %s
-// expected-no-diagnostics
+// RUN: %clang_cc1 -DDIGRAPHS=1 -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -DDIGRAPHS=1 -fno-digraphs -fdigraphs -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -fno-digraphs -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -fdigraphs -fno-digraphs -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -std=c89 -DDIGRAPHS=1 -fdigraphs -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -std=c89 -fno-digraphs -fsyntax-only -verify -ffreestanding %s
+
+#if DIGRAPHS
 
+// expected-no-diagnostics
 %:include 
 
 %:ifndef BUFSIZE
@@ -14,3 +21,15 @@
 d<:len:> = s<:len:>;
 %>
 %>
+#else
+
+// expected-error@+1 {{expected identifier or '('}}
+%:include 
+;
+// expected-error@+1 {{expected ')'}} expected-note@+1{{to match this '('}}
+void copy(char d<::>);
+
+// expected-error@+1 {{expected function body}}
+void copy() <% %>
+
+#endif
Index: lib/Frontend/CompilerInvocation.cpp
===
--- 

[PATCH] D48266: [Driver] Add -fno-digraphs

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes marked 2 inline comments as done.
jtbandes added a comment.

If I understood your comment correctly, you meant to remove the diagnostic 
completely (regardless of whether `-fdigraphs` or `-fno-digraphs` is given, and 
regardless of whether digraphs were enabled for the selected language)? I've 
done that, and added a couple `-std=c89` invocations to the test cases.


Repository:
  rC Clang

https://reviews.llvm.org/D48266



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

[PATCH] D48266: [Driver] Add -fno-digraphs

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes updated this revision to Diff 155115.
jtbandes added a comment.

- remove diagnostic; fix formatting


Repository:
  rC Clang

https://reviews.llvm.org/D48266

Files:
  include/clang/Driver/Options.td
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/Lexer/digraph.c


Index: test/Lexer/digraph.c
===
--- test/Lexer/digraph.c
+++ test/Lexer/digraph.c
@@ -1,6 +1,13 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -ffreestanding %s
-// expected-no-diagnostics
+// RUN: %clang_cc1 -DDIGRAPHS=1 -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -DDIGRAPHS=1 -fno-digraphs -fdigraphs -fsyntax-only -verify 
-ffreestanding %s
+// RUN: %clang_cc1 -fno-digraphs -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -fdigraphs -fno-digraphs -fsyntax-only -verify 
-ffreestanding %s
+// RUN: %clang_cc1 -std=c89 -DDIGRAPHS=1 -fdigraphs -fsyntax-only -verify 
-ffreestanding %s
+// RUN: %clang_cc1 -std=c89 -fno-digraphs -fsyntax-only -verify -ffreestanding 
%s
+
+#if DIGRAPHS
 
+// expected-no-diagnostics
 %:include 
 
 %:ifndef BUFSIZE
@@ -14,3 +21,15 @@
 d<:len:> = s<:len:>;
 %>
 %>
+#else
+
+// expected-error@+1 {{expected identifier or '('}}
+%:include 
+;
+// expected-error@+1 {{expected ')'}} expected-note@+1{{to match this '('}}
+void copy(char d<::>);
+
+// expected-error@+1 {{expected function body}}
+void copy() <% %>
+
+#endif
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2174,6 +2174,9 @@
   Opts.GNUKeywords = Args.hasFlag(OPT_fgnu_keywords, OPT_fno_gnu_keywords,
   Opts.GNUKeywords);
 
+  if (const Arg *A = Args.getLastArg(OPT_fdigraphs, OPT_fno_digraphs))
+Opts.Digraphs = A->getOption().matches(OPT_fdigraphs) ? 1 : 0;
+
   if (Args.hasArg(OPT_fno_operator_names))
 Opts.CXXOperatorNames = 0;
 
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3970,6 +3970,7 @@
   // Forward -f (flag) options which we can pass directly.
   Args.AddLastArg(CmdArgs, options::OPT_femit_all_decls);
   Args.AddLastArg(CmdArgs, options::OPT_fheinous_gnu_extensions);
+  Args.AddLastArg(CmdArgs, options::OPT_fdigraphs, options::OPT_fno_digraphs);
   Args.AddLastArg(CmdArgs, options::OPT_fno_operator_names);
   Args.AddLastArg(CmdArgs, options::OPT_femulated_tls,
   options::OPT_fno_emulated_tls);
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1334,6 +1334,10 @@
 def fno_diagnostics_show_option : Flag<["-"], "fno-diagnostics-show-option">, 
Group;
 def fno_diagnostics_show_note_include_stack : Flag<["-"], 
"fno-diagnostics-show-note-include-stack">,
 Flags<[CC1Option]>, Group;
+def fdigraphs : Flag<["-"], "fdigraphs">, Group, Flags<[CC1Option]>,
+  HelpText<"Enable alternative token representations '<:', ':>', '<%', '%>', 
'%:' (default)">;
+def fno_digraphs : Flag<["-"], "fno-digraphs">, Group, 
Flags<[CC1Option]>,
+  HelpText<"Disallow alternative token representations '<:', ':>', '<%', '%>', 
'%:'">;
 def fno_declspec : Flag<["-"], "fno-declspec">, Group,
   HelpText<"Disallow __declspec as a keyword">, Flags<[CC1Option]>;
 def fno_dollars_in_identifiers : Flag<["-"], "fno-dollars-in-identifiers">, 
Group,


Index: test/Lexer/digraph.c
===
--- test/Lexer/digraph.c
+++ test/Lexer/digraph.c
@@ -1,6 +1,13 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -ffreestanding %s
-// expected-no-diagnostics
+// RUN: %clang_cc1 -DDIGRAPHS=1 -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -DDIGRAPHS=1 -fno-digraphs -fdigraphs -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -fno-digraphs -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -fdigraphs -fno-digraphs -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -std=c89 -DDIGRAPHS=1 -fdigraphs -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -std=c89 -fno-digraphs -fsyntax-only -verify -ffreestanding %s
+
+#if DIGRAPHS
 
+// expected-no-diagnostics
 %:include 
 
 %:ifndef BUFSIZE
@@ -14,3 +21,15 @@
 d<:len:> = s<:len:>;
 %>
 %>
+#else
+
+// expected-error@+1 {{expected identifier or '('}}
+%:include 
+;
+// expected-error@+1 {{expected ')'}} expected-note@+1{{to match this '('}}
+void copy(char d<::>);
+
+// expected-error@+1 {{expected function body}}
+void copy() <% %>
+
+#endif
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ 

[PATCH] D48266: [Driver] Add -fno-digraphs

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added a comment.

Friendly ping!


Repository:
  rC Clang

https://reviews.llvm.org/D48266



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

[PATCH] D48266: [Driver] Add -fno-digraphs

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes updated this revision to Diff 151858.
jtbandes marked an inline comment as done.
jtbandes added a comment.

Added an error when language standard doesn't support digraphs.

Still keeping `-fdigraphs` as a cc1 option because then I can distinguish 
explicitly-enabled/disabled from the absence of a flag. I can also check 
whether digraphs are supported using the LangStandard/LangOpts in the 
CompilerInstance rather than hard-coding an incompatibility with -std=c89.


Repository:
  rC Clang

https://reviews.llvm.org/D48266

Files:
  include/clang/Driver/Options.td
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/Lexer/digraph.c

Index: test/Lexer/digraph.c
===
--- test/Lexer/digraph.c
+++ test/Lexer/digraph.c
@@ -1,6 +1,17 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -ffreestanding %s
-// expected-no-diagnostics
+// RUN: %clang_cc1 -DDIGRAPHS=1 -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -DDIGRAPHS=1 -fno-digraphs -fdigraphs -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -fno-digraphs -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -fdigraphs -fno-digraphs -fsyntax-only -verify -ffreestanding %s
+
+// RUN: not %clang_cc1 -std=c89 -fdigraphs -fsyntax-only -ffreestanding %s 2>&1 | FileCheck -check-prefix=CHECK1 %s
+// RUN: not %clang_cc1 -std=c89 -fno-digraphs -fsyntax-only -ffreestanding %s 2>&1 | FileCheck -check-prefix=CHECK2 %s
+
+// CHECK1: error: invalid argument '-fdigraphs' not allowed with '-std=c89'
+// CHECK2: error: invalid argument '-fno-digraphs' not allowed with '-std=c89'
 
+#if DIGRAPHS
+
+// expected-no-diagnostics
 %:include 
 
 %:ifndef BUFSIZE
@@ -14,3 +25,15 @@
 d<:len:> = s<:len:>;
 %>
 %>
+#else
+
+// expected-error@+1 {{expected identifier or '('}}
+%:include 
+;
+// expected-error@+1 {{expected ')'}} expected-note@+1{{to match this '('}}
+void copy(char d<::>);
+
+// expected-error@+1 {{expected function body}}
+void copy() <% %>
+
+#endif
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2080,6 +2080,7 @@
   DiagnosticsEngine ) {
   // FIXME: Cleanup per-file based stuff.
   LangStandard::Kind LangStd = LangStandard::lang_unspecified;
+  const Arg *LangStdArg;
   if (const Arg *A = Args.getLastArg(OPT_std_EQ)) {
 LangStd = llvm::StringSwitch(A->getValue())
 #define LANGSTANDARD(id, name, lang, desc, features) \
@@ -2117,6 +2118,7 @@
 } else {
   // Valid standard, check to make sure language and standard are
   // compatible.
+  LangStdArg = A;
   const LangStandard  = LangStandard::getLangStandardForKind(LangStd);
   if (!IsInputCompatibleWithStandard(IK, Std)) {
 Diags.Report(diag::err_drv_argument_not_allowed_with)
@@ -2147,8 +2149,10 @@
   Diags.Report(diag::err_drv_invalid_value)
 << A->getAsString(Args) << A->getValue();
 }
-else
+else {
   LangStd = OpenCLLangStd;
+  LangStdArg = A;
+}
   }
 
   Opts.IncludeDefaultHeader = Args.hasArg(OPT_finclude_default_header);
@@ -2174,6 +2178,16 @@
   Opts.GNUKeywords = Args.hasFlag(OPT_fgnu_keywords, OPT_fno_gnu_keywords,
   Opts.GNUKeywords);
 
+  if (const Arg* A = Args.getLastArg(OPT_fdigraphs, OPT_fno_digraphs)) {
+// Prevent the user from enabling or disabling digraphs when they are not supported.
+if (!Opts.Digraphs && LangStdArg)
+  Diags.Report(diag::err_drv_argument_not_allowed_with)
+  << A->getSpelling() << LangStdArg->getAsString(Args);
+
+if (A->getOption().matches(OPT_fno_digraphs))
+  Opts.Digraphs = 0;
+  }
+
   if (Args.hasArg(OPT_fno_operator_names))
 Opts.CXXOperatorNames = 0;
 
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3970,6 +3970,7 @@
   // Forward -f (flag) options which we can pass directly.
   Args.AddLastArg(CmdArgs, options::OPT_femit_all_decls);
   Args.AddLastArg(CmdArgs, options::OPT_fheinous_gnu_extensions);
+  Args.AddLastArg(CmdArgs, options::OPT_fdigraphs, options::OPT_fno_digraphs);
   Args.AddLastArg(CmdArgs, options::OPT_fno_operator_names);
   Args.AddLastArg(CmdArgs, options::OPT_femulated_tls,
   options::OPT_fno_emulated_tls);
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1334,6 +1334,10 @@
 def fno_diagnostics_show_option : Flag<["-"], "fno-diagnostics-show-option">, Group;
 def fno_diagnostics_show_note_include_stack : Flag<["-"], "fno-diagnostics-show-note-include-stack">,
 Flags<[CC1Option]>, Group;
+def fdigraphs : Flag<["-"], 

[PATCH] D48266: [Driver] Add -fno-digraphs

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes updated this revision to Diff 151771.
jtbandes added a comment.

Added `-fdigraphs`. I kept it as a cc1 option because it seemed awkward to 
"check whether the last arg was -fno-digraphs and pass only that arg to cc1" 
(if just directly forwarding all args, there would be an unrecognized argument 
error if it's not a cc1 option).


Repository:
  rC Clang

https://reviews.llvm.org/D48266

Files:
  include/clang/Driver/Options.td
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/Lexer/digraph.c


Index: test/Lexer/digraph.c
===
--- test/Lexer/digraph.c
+++ test/Lexer/digraph.c
@@ -1,6 +1,11 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -ffreestanding %s
-// expected-no-diagnostics
+// RUN: %clang_cc1 -DDIGRAPHS -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -DDIGRAPHS -fno-digraphs -fdigraphs -fsyntax-only -verify 
-ffreestanding %s
+// RUN: %clang_cc1 -fno-digraphs -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -fdigraphs -fno-digraphs -fsyntax-only -verify 
-ffreestanding %s
+
+#if DIGRAPHS
 
+// expected-no-diagnostics
 %:include 
 
 %:ifndef BUFSIZE
@@ -14,3 +19,15 @@
 d<:len:> = s<:len:>;
 %>
 %>
+#else
+
+// expected-error@+1 {{expected identifier or '('}}
+%:include 
+;
+// expected-error@+1 {{expected ')'}} expected-note@+1{{to match this '('}}
+void copy(char d<::>);
+
+// expected-error@+1 {{expected function body}}
+void copy() <% %>
+
+#endif
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2174,6 +2174,8 @@
   Opts.GNUKeywords = Args.hasFlag(OPT_fgnu_keywords, OPT_fno_gnu_keywords,
   Opts.GNUKeywords);
 
+  Opts.Digraphs = Args.hasFlag(OPT_fdigraphs, OPT_fno_digraphs, Opts.Digraphs);
+
   if (Args.hasArg(OPT_fno_operator_names))
 Opts.CXXOperatorNames = 0;
 
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3970,6 +3970,7 @@
   // Forward -f (flag) options which we can pass directly.
   Args.AddLastArg(CmdArgs, options::OPT_femit_all_decls);
   Args.AddLastArg(CmdArgs, options::OPT_fheinous_gnu_extensions);
+  Args.AddLastArg(CmdArgs, options::OPT_fdigraphs, options::OPT_fno_digraphs);
   Args.AddLastArg(CmdArgs, options::OPT_fno_operator_names);
   Args.AddLastArg(CmdArgs, options::OPT_femulated_tls,
   options::OPT_fno_emulated_tls);
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1334,6 +1334,10 @@
 def fno_diagnostics_show_option : Flag<["-"], "fno-diagnostics-show-option">, 
Group;
 def fno_diagnostics_show_note_include_stack : Flag<["-"], 
"fno-diagnostics-show-note-include-stack">,
 Flags<[CC1Option]>, Group;
+def fdigraphs : Flag<["-"], "fdigraphs">, Group, Flags<[CC1Option]>,
+  HelpText<"Enable alternative token representations '<:', ':>', '<%', '%>', 
'%:' (default)">;
+def fno_digraphs : Flag<["-"], "fno-digraphs">, Group, 
Flags<[CC1Option]>,
+  HelpText<"Disallow alternative token representations '<:', ':>', '<%', '%>', 
'%:'">;
 def fno_declspec : Flag<["-"], "fno-declspec">, Group,
   HelpText<"Disallow __declspec as a keyword">, Flags<[CC1Option]>;
 def fno_dollars_in_identifiers : Flag<["-"], "fno-dollars-in-identifiers">, 
Group,


Index: test/Lexer/digraph.c
===
--- test/Lexer/digraph.c
+++ test/Lexer/digraph.c
@@ -1,6 +1,11 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -ffreestanding %s
-// expected-no-diagnostics
+// RUN: %clang_cc1 -DDIGRAPHS -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -DDIGRAPHS -fno-digraphs -fdigraphs -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -fno-digraphs -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -fdigraphs -fno-digraphs -fsyntax-only -verify -ffreestanding %s
+
+#if DIGRAPHS
 
+// expected-no-diagnostics
 %:include 
 
 %:ifndef BUFSIZE
@@ -14,3 +19,15 @@
 d<:len:> = s<:len:>;
 %>
 %>
+#else
+
+// expected-error@+1 {{expected identifier or '('}}
+%:include 
+;
+// expected-error@+1 {{expected ')'}} expected-note@+1{{to match this '('}}
+void copy(char d<::>);
+
+// expected-error@+1 {{expected function body}}
+void copy() <% %>
+
+#endif
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2174,6 +2174,8 @@
   Opts.GNUKeywords = Args.hasFlag(OPT_fgnu_keywords, OPT_fno_gnu_keywords,
   Opts.GNUKeywords);
 
+  Opts.Digraphs = 

[PATCH] D48266: [Driver] Add -fno-digraphs

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes marked an inline comment as done.
jtbandes added inline comments.



Comment at: include/clang/Driver/Options.td:1337-1338
 Flags<[CC1Option]>, Group;
+def fno_digraphs : Flag<["-"], "fno-digraphs">, Group, 
Flags<[CC1Option]>,
+  HelpText<"Disallow alternative token representations '<:', ':>', '<%', '%>', 
'%:'">;
 def fno_declspec : Flag<["-"], "fno-declspec">, Group,

rsmith wrote:
> In the driver, we generally want a `-ffoo` option matching `-fno-foo`. That 
> is, the driver (but not `-cc1`) should support a matching `-fdigraphs` option 
> to undo the effect of `-fno-digraphs`, unless there's a good reason not to do 
> so.
Didn't find a great place to put this option; I'm not sure what this file's 
organization scheme is supposed to be.


Repository:
  rC Clang

https://reviews.llvm.org/D48266



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

[PATCH] D48266: [Driver] Add -fno-digraphs

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes created this revision.

Add a flag `-fno-digraphs` to disable digraphs in the lexer, similar to 
`-fno-operator-names` which disables alternative names for C++ operators.


Repository:
  rC Clang

https://reviews.llvm.org/D48266

Files:
  include/clang/Driver/Options.td
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/Lexer/digraph.c


Index: test/Lexer/digraph.c
===
--- test/Lexer/digraph.c
+++ test/Lexer/digraph.c
@@ -1,6 +1,9 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -ffreestanding %s
-// expected-no-diagnostics
+// RUN: %clang_cc1 -DDIGRAPHS -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -fno-digraphs -fsyntax-only -verify -ffreestanding %s
+
+#if DIGRAPHS
 
+// expected-no-diagnostics
 %:include 
 
 %:ifndef BUFSIZE
@@ -14,3 +17,15 @@
 d<:len:> = s<:len:>;
 %>
 %>
+#else
+
+// expected-error@+1 {{expected identifier or '('}}
+%:include 
+;
+// expected-error@+1 {{expected ')'}} expected-note@+1{{to match this '('}}
+void copy(char d<::>);
+
+// expected-error@+1 {{expected function body}}
+void copy() <% %>
+
+#endif
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2174,6 +2174,9 @@
   Opts.GNUKeywords = Args.hasFlag(OPT_fgnu_keywords, OPT_fno_gnu_keywords,
   Opts.GNUKeywords);
 
+  if (Args.hasArg(OPT_fno_digraphs))
+Opts.Digraphs = 0;
+
   if (Args.hasArg(OPT_fno_operator_names))
 Opts.CXXOperatorNames = 0;
 
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3970,6 +3970,7 @@
   // Forward -f (flag) options which we can pass directly.
   Args.AddLastArg(CmdArgs, options::OPT_femit_all_decls);
   Args.AddLastArg(CmdArgs, options::OPT_fheinous_gnu_extensions);
+  Args.AddLastArg(CmdArgs, options::OPT_fno_digraphs);
   Args.AddLastArg(CmdArgs, options::OPT_fno_operator_names);
   Args.AddLastArg(CmdArgs, options::OPT_femulated_tls,
   options::OPT_fno_emulated_tls);
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1334,6 +1334,8 @@
 def fno_diagnostics_show_option : Flag<["-"], "fno-diagnostics-show-option">, 
Group;
 def fno_diagnostics_show_note_include_stack : Flag<["-"], 
"fno-diagnostics-show-note-include-stack">,
 Flags<[CC1Option]>, Group;
+def fno_digraphs : Flag<["-"], "fno-digraphs">, Group, 
Flags<[CC1Option]>,
+  HelpText<"Disallow alternative token representations '<:', ':>', '<%', '%>', 
'%:'">;
 def fno_declspec : Flag<["-"], "fno-declspec">, Group,
   HelpText<"Disallow __declspec as a keyword">, Flags<[CC1Option]>;
 def fno_dollars_in_identifiers : Flag<["-"], "fno-dollars-in-identifiers">, 
Group,


Index: test/Lexer/digraph.c
===
--- test/Lexer/digraph.c
+++ test/Lexer/digraph.c
@@ -1,6 +1,9 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -ffreestanding %s
-// expected-no-diagnostics
+// RUN: %clang_cc1 -DDIGRAPHS -fsyntax-only -verify -ffreestanding %s
+// RUN: %clang_cc1 -fno-digraphs -fsyntax-only -verify -ffreestanding %s
+
+#if DIGRAPHS
 
+// expected-no-diagnostics
 %:include 
 
 %:ifndef BUFSIZE
@@ -14,3 +17,15 @@
 d<:len:> = s<:len:>;
 %>
 %>
+#else
+
+// expected-error@+1 {{expected identifier or '('}}
+%:include 
+;
+// expected-error@+1 {{expected ')'}} expected-note@+1{{to match this '('}}
+void copy(char d<::>);
+
+// expected-error@+1 {{expected function body}}
+void copy() <% %>
+
+#endif
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2174,6 +2174,9 @@
   Opts.GNUKeywords = Args.hasFlag(OPT_fgnu_keywords, OPT_fno_gnu_keywords,
   Opts.GNUKeywords);
 
+  if (Args.hasArg(OPT_fno_digraphs))
+Opts.Digraphs = 0;
+
   if (Args.hasArg(OPT_fno_operator_names))
 Opts.CXXOperatorNames = 0;
 
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3970,6 +3970,7 @@
   // Forward -f (flag) options which we can pass directly.
   Args.AddLastArg(CmdArgs, options::OPT_femit_all_decls);
   Args.AddLastArg(CmdArgs, options::OPT_fheinous_gnu_extensions);
+  Args.AddLastArg(CmdArgs, options::OPT_fno_digraphs);
   Args.AddLastArg(CmdArgs, options::OPT_fno_operator_names);
   Args.AddLastArg(CmdArgs, options::OPT_femulated_tls,
   options::OPT_fno_emulated_tls);

[PATCH] D40284: [Sema] Improve diagnostics for template arg deduction

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added a comment.

Please take another look when you get a chance :)


Repository:
  rC Clang

https://reviews.llvm.org/D40284



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

[PATCH] D40284: [Sema] Improve diagnostics for template arg deduction

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes updated this revision to Diff 133440.
jtbandes added a comment.

Using a slightly more invasive fix. I haven't come up with any other test cases 
that exhibit the problem, which makes me unsure this fix is needed in all these 
locations. Maybe someone with more knowledge of this function can advise.


Repository:
  rC Clang

https://reviews.llvm.org/D40284

Files:
  lib/Sema/SemaTemplateDeduction.cpp
  test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/basic.cpp

Index: test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/basic.cpp
===
--- test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/basic.cpp
+++ test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/basic.cpp
@@ -45,3 +45,11 @@
 func(foo()); // expected-error {{no matching function}}
   }
 }
+
+namespace test4 {
+  // expected-note@+1 {{candidate template ignored: could not match 'int [N]' against 'int []'}}
+  template void f1(int ()[N]);
+  template void f2(int ()[]) {
+f1(arr); // expected-error {{no matching function}}
+  }
+}
Index: lib/Sema/SemaTemplateDeduction.cpp
===
--- lib/Sema/SemaTemplateDeduction.cpp
+++ lib/Sema/SemaTemplateDeduction.cpp
@@ -1289,27 +1289,34 @@
 return Sema::TDK_Success;
   }
 
-  // Set up the template argument deduction information for a failure.
-  Info.FirstArg = TemplateArgument(ParamIn);
-  Info.SecondArg = TemplateArgument(ArgIn);
-
   // If the parameter is an already-substituted template parameter
   // pack, do nothing: we don't know which of its arguments to look
   // at, so we have to wait until all of the parameter packs in this
   // expansion have arguments.
   if (isa(Param))
 return Sema::TDK_Success;
 
+  // Use this when returning a failure result in order to fill in the
+  // corresponding failure info. Don't use this when returning the
+  // result of a recursive call, as the callee will have already set
+  // their own failure info.
+  const auto WithFailureInfo =
+[, , ](Sema::TemplateDeductionResult Result) {
+  Info.FirstArg = TemplateArgument(ParamIn);
+  Info.SecondArg = TemplateArgument(ArgIn);
+  return Result;
+};
+
   // Check the cv-qualifiers on the parameter and argument types.
   CanQualType CanParam = S.Context.getCanonicalType(Param);
   CanQualType CanArg = S.Context.getCanonicalType(Arg);
   if (!(TDF & TDF_IgnoreQualifiers)) {
 if (TDF & TDF_ParamWithReferenceType) {
   if (hasInconsistentOrSupersetQualifiersOf(Param, Arg))
-return Sema::TDK_NonDeducedMismatch;
+return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
 } else if (!IsPossiblyOpaquelyQualifiedType(Param)) {
   if (Param.getCVRQualifiers() != Arg.getCVRQualifiers())
-return Sema::TDK_NonDeducedMismatch;
+return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
 }
 
 // If the parameter type is not dependent, there is nothing to deduce.
@@ -1319,9 +1326,8 @@
 (TDF & TDF_AllowCompatibleFunctionType)
 ? !S.isSameOrCompatibleFunctionType(CanParam, CanArg)
 : Param != Arg;
-if (NonDeduced) {
-  return Sema::TDK_NonDeducedMismatch;
-}
+if (NonDeduced)
+  return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
   }
   return Sema::TDK_Success;
 }
@@ -1366,7 +1372,10 @@
 Arg = Arg.getUnqualifiedType();
   }
 
-  return Param == Arg? Sema::TDK_Success : Sema::TDK_NonDeducedMismatch;
+  if (Param == Arg)
+return Sema::TDK_Success;
+
+  return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
 }
 
 // _Complex T   [placeholder extension]
@@ -1377,7 +1386,7 @@
 ComplexArg->getElementType(),
 Info, Deduced, TDF);
 
-  return Sema::TDK_NonDeducedMismatch;
+  return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
 
 // _Atomic T   [extension]
 case Type::Atomic:
@@ -1387,7 +1396,7 @@
AtomicArg->getValueType(),
Info, Deduced, TDF);
 
-  return Sema::TDK_NonDeducedMismatch;
+  return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
 
 // T *
 case Type::Pointer: {
@@ -1398,7 +1407,7 @@
= Arg->getAs()) {
 PointeeType = PointerArg->getPointeeType();
   } else {
-return Sema::TDK_NonDeducedMismatch;
+return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
   }
 
   unsigned SubTDF = TDF & (TDF_IgnoreQualifiers | TDF_DerivedClass);
@@ -1413,7 +1422,7 @@
   const LValueReferenceType *ReferenceArg =
   Arg->getAs();
   if (!ReferenceArg)
-return Sema::TDK_NonDeducedMismatch;
+return WithFailureInfo(Sema::TDK_NonDeducedMismatch);
 
   return DeduceTemplateArgumentsByTypeMatch(S, 

[PATCH] D41646: [Sema] Improve diagnostics for const- and ref-qualified member functions

[Jacob Bandes-Storch via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL321609: [Sema] Improve diagnostics for const- and 
ref-qualified member functions (authored by jtbandes, committed by ).

Repository:
  rL LLVM

https://reviews.llvm.org/D41646

Files:
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/Sema/SemaOverload.cpp
  cfe/trunk/test/CXX/over/over.match/over.match.funcs/p4-0x.cpp
  cfe/trunk/test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp
  cfe/trunk/test/SemaCXX/copy-initialization.cpp
  cfe/trunk/test/SemaCXX/cxx1y-contextual-conversion-tweaks.cpp
  cfe/trunk/test/SemaCXX/cxx2a-pointer-to-const-ref-member.cpp

Index: cfe/trunk/lib/Sema/SemaOverload.cpp
===
--- cfe/trunk/lib/Sema/SemaOverload.cpp
+++ cfe/trunk/lib/Sema/SemaOverload.cpp
@@ -5145,7 +5145,8 @@
   *this, From->getLocStart(), From->getType(), FromClassification, Method,
   Method->getParent());
   if (ICS.isBad()) {
-if (ICS.Bad.Kind == BadConversionSequence::bad_qualifiers) {
+switch (ICS.Bad.Kind) {
+case BadConversionSequence::bad_qualifiers: {
   Qualifiers FromQs = FromRecordType.getQualifiers();
   Qualifiers ToQs = DestType.getQualifiers();
   unsigned CVR = FromQs.getCVRQualifiers() & ~ToQs.getCVRQualifiers();
@@ -5158,10 +5159,28 @@
   << Method->getDeclName();
 return ExprError();
   }
+  break;
+}
+
+case BadConversionSequence::lvalue_ref_to_rvalue:
+case BadConversionSequence::rvalue_ref_to_lvalue: {
+  bool IsRValueQualified =
+Method->getRefQualifier() == RefQualifierKind::RQ_RValue;
+  Diag(From->getLocStart(), diag::err_member_function_call_bad_ref)
+<< Method->getDeclName() << FromClassification.isRValue()
+<< IsRValueQualified;
+  Diag(Method->getLocation(), diag::note_previous_decl)
+<< Method->getDeclName();
+  return ExprError();
+}
+
+case BadConversionSequence::no_conversion:
+case BadConversionSequence::unrelated_class:
+  break;
 }
 
 return Diag(From->getLocStart(),
-diag::err_implicit_object_parameter_init)
+diag::err_member_function_call_bad_type)
<< ImplicitParamRecordType << FromRecordType << From->getSourceRange();
   }
 
Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -1589,12 +1589,20 @@
 def ext_pure_function_definition : ExtWarn<
   "function definition with pure-specifier is a Microsoft extension">,
   InGroup;
-def err_implicit_object_parameter_init : Error<
-  "cannot initialize object parameter of type %0 with an expression "
-  "of type %1">;
 def err_qualified_member_of_unrelated : Error<
   "%q0 is not a member of class %1">;
 
+def err_member_function_call_bad_cvr : Error<
+  "'this' argument to member function %0 has type %1, but function is not marked "
+  "%select{const|restrict|const or restrict|volatile|const or volatile|"
+  "volatile or restrict|const, volatile, or restrict}2">;
+def err_member_function_call_bad_ref : Error<
+  "'this' argument to member function %0 is an %select{lvalue|rvalue}1, "
+  "but function has %select{non-const lvalue|rvalue}2 ref-qualifier">;
+def err_member_function_call_bad_type : Error<
+  "cannot initialize object parameter of type %0 with an expression "
+  "of type %1">;
+
 def warn_call_to_pure_virtual_member_function_from_ctor_dtor : Warning<
   "call to pure virtual member function %0 has undefined behavior; "
   "overrides of %0 in subclasses are not available in the "
@@ -1815,10 +1823,6 @@
 def err_init_list_bad_dest_type : Error<
   "%select{|non-aggregate }0type %1 cannot be initialized with an initializer "
   "list">;
-def err_member_function_call_bad_cvr : Error<"member function %0 not viable: "
-"'this' argument has type %1, but function is not marked "
-"%select{const|restrict|const or restrict|volatile|const or volatile|"
-"volatile or restrict|const, volatile, or restrict}2">;
 
 def err_reference_bind_to_bitfield : Error<
   "%select{non-const|volatile}0 reference cannot bind to "
Index: cfe/trunk/test/SemaCXX/cxx2a-pointer-to-const-ref-member.cpp
===
--- cfe/trunk/test/SemaCXX/cxx2a-pointer-to-const-ref-member.cpp
+++ cfe/trunk/test/SemaCXX/cxx2a-pointer-to-const-ref-member.cpp
@@ -1,12 +1,12 @@
 // RUN: %clang_cc1 -std=c++2a %s -verify
 
 struct X {
-  void ref() & {}
+  void ref() & {} // expected-note{{'ref' declared here}}
   void cref() const& {}
 };
 
 void test() {
-  X{}.ref(); // expected-error{{cannot initialize object parameter of type 'X' with an expression of type 'X'}}
+  X{}.ref(); // expected-error{{'this' argument to member function 'ref' is an rvalue, but function has 

[PATCH] D39937: [Sema] Improve diagnostics for const- and ref-qualified member functions

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added a comment.

After merging the buildbots informed me some other tests were broken that I 
failed to notice. I reverted this commit and submitted 
https://reviews.llvm.org/D41646 which reintroduces the changes and fixes the 
other broken tests.


Repository:
  rC Clang

https://reviews.llvm.org/D39937



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

[PATCH] D41646: [Sema] Improve diagnostics for const- and ref-qualified member functions

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes created this revision.
jtbandes added a reviewer: aaron.ballman.

Re-submission of https://reviews.llvm.org/D39937 with additional fixed tests.


Repository:
  rC Clang

https://reviews.llvm.org/D41646

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaOverload.cpp
  test/CXX/over/over.match/over.match.funcs/p4-0x.cpp
  test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp
  test/SemaCXX/copy-initialization.cpp
  test/SemaCXX/cxx1y-contextual-conversion-tweaks.cpp
  test/SemaCXX/cxx2a-pointer-to-const-ref-member.cpp

Index: test/SemaCXX/cxx2a-pointer-to-const-ref-member.cpp
===
--- test/SemaCXX/cxx2a-pointer-to-const-ref-member.cpp
+++ test/SemaCXX/cxx2a-pointer-to-const-ref-member.cpp
@@ -1,12 +1,12 @@
 // RUN: %clang_cc1 -std=c++2a %s -verify
 
 struct X {
-  void ref() & {}
+  void ref() & {} // expected-note{{'ref' declared here}}
   void cref() const& {}
 };
 
 void test() {
-  X{}.ref(); // expected-error{{cannot initialize object parameter of type 'X' with an expression of type 'X'}}
+  X{}.ref(); // expected-error{{'this' argument to member function 'ref' is an rvalue, but function has non-const lvalue ref-qualifier}}
   X{}.cref(); // expected-no-error
 
   (X{}.*::ref)(); // expected-error-re{{pointer-to-member function type 'void (X::*)() {{.*}}&' can only be called on an lvalue}}
Index: test/SemaCXX/cxx1y-contextual-conversion-tweaks.cpp
===
--- test/SemaCXX/cxx1y-contextual-conversion-tweaks.cpp
+++ test/SemaCXX/cxx1y-contextual-conversion-tweaks.cpp
@@ -96,8 +96,8 @@
 //expected-error@81 {{statement requires expression of integer type ('extended_examples::A1' invalid)}}
 //expected-error@85 {{statement requires expression of integer type ('extended_examples::B2' invalid)}}
 #else
-//expected-error@81 {{cannot initialize object parameter of type 'extended_examples::A1' with an expression of type 'extended_examples::A1'}}
-//expected-error@85 {{cannot initialize object parameter of type 'extended_examples::B2' with an expression of type 'extended_examples::B2'}}
+//expected-error@81 {{'this' argument to member function 'operator int' is an lvalue, but function has rvalue ref-qualifier}} expected-note@54 {{'operator int' declared here}}
+//expected-error@85 {{'this' argument to member function 'operator int' is an lvalue, but function has rvalue ref-qualifier}} expected-note@75 {{'operator int' declared here}}
 #endif
 
 namespace extended_examples_cxx1y {
@@ -149,9 +149,9 @@
 #ifdef CXX1Y
 //expected-error@139 {{statement requires expression of integer type ('extended_examples_cxx1y::A2' invalid)}}
 #else
-//expected-error@138 {{cannot initialize object parameter of type 'extended_examples_cxx1y::A1' with an expression of type 'extended_examples_cxx1y::A1'}}
-//expected-error@139 {{cannot initialize object parameter of type 'extended_examples_cxx1y::A2' with an expression of type 'extended_examples_cxx1y::A2'}}
-//expected-error@143 {{cannot initialize object parameter of type 'extended_examples_cxx1y::D' with an expression of type 'extended_examples_cxx1y::D'}}
+//expected-error@138 {{'this' argument to member function 'operator int' is an lvalue, but function has rvalue ref-qualifier}} expected-note@106 {{'operator int' declared here}}
+//expected-error@139 {{'this' argument to member function 'operator int' is an lvalue, but function has rvalue ref-qualifier}} expected-note@111 {{'operator int' declared here}}
+//expected-error@143 {{'this' argument to member function 'operator int' is an lvalue, but function has rvalue ref-qualifier}} expected-note@131 {{'operator int' declared here}}
 #endif
 
 namespace extended_examples_array_bounds {
Index: test/SemaCXX/copy-initialization.cpp
===
--- test/SemaCXX/copy-initialization.cpp
+++ test/SemaCXX/copy-initialization.cpp
@@ -26,7 +26,7 @@
 };
 
 // PR3600
-void test(const foo *P) { P->bar(); } // expected-error{{'bar' not viable: 'this' argument has type 'const foo', but function is not marked const}}
+void test(const foo *P) { P->bar(); } // expected-error{{'this' argument to member function 'bar' has type 'const foo', but function is not marked const}}
 
 namespace PR6757 {
   struct Foo {
Index: test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp
===
--- test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp
+++ test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp
@@ -215,7 +215,7 @@
 template
 void i(T t) {
   for (auto u : t) { // expected-error {{invalid range expression of type 'X::A *'; no viable 'begin' function available}} \
-expected-error {{member function 'begin' not viable}} \
+expected-error {{'this' argument to member function 'begin' has type 'const X::A', but function is not marked const}} \
 

[PATCH] D39937: [Sema] Improve diagnostics for const- and ref-qualified member functions

[Jacob Bandes-Storch via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC321592: [Sema] Improve diagnostics for const- and 
ref-qualified member functions (authored by jtbandes, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D39937?vs=123481=128366#toc

Repository:
  rC Clang

https://reviews.llvm.org/D39937

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaOverload.cpp
  test/CXX/over/over.match/over.match.funcs/p4-0x.cpp
  test/SemaCXX/copy-initialization.cpp

Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -1589,12 +1589,20 @@
 def ext_pure_function_definition : ExtWarn<
   "function definition with pure-specifier is a Microsoft extension">,
   InGroup;
-def err_implicit_object_parameter_init : Error<
-  "cannot initialize object parameter of type %0 with an expression "
-  "of type %1">;
 def err_qualified_member_of_unrelated : Error<
   "%q0 is not a member of class %1">;
 
+def err_member_function_call_bad_cvr : Error<
+  "'this' argument to member function %0 has type %1, but function is not marked "
+  "%select{const|restrict|const or restrict|volatile|const or volatile|"
+  "volatile or restrict|const, volatile, or restrict}2">;
+def err_member_function_call_bad_ref : Error<
+  "'this' argument to member function %0 is an %select{lvalue|rvalue}1, "
+  "but function has %select{non-const lvalue|rvalue}2 ref-qualifier">;
+def err_member_function_call_bad_type : Error<
+  "cannot initialize object parameter of type %0 with an expression "
+  "of type %1">;
+
 def warn_call_to_pure_virtual_member_function_from_ctor_dtor : Warning<
   "call to pure virtual member function %0 has undefined behavior; "
   "overrides of %0 in subclasses are not available in the "
@@ -1815,10 +1823,6 @@
 def err_init_list_bad_dest_type : Error<
   "%select{|non-aggregate }0type %1 cannot be initialized with an initializer "
   "list">;
-def err_member_function_call_bad_cvr : Error<"member function %0 not viable: "
-"'this' argument has type %1, but function is not marked "
-"%select{const|restrict|const or restrict|volatile|const or volatile|"
-"volatile or restrict|const, volatile, or restrict}2">;
 
 def err_reference_bind_to_bitfield : Error<
   "%select{non-const|volatile}0 reference cannot bind to "
Index: test/SemaCXX/copy-initialization.cpp
===
--- test/SemaCXX/copy-initialization.cpp
+++ test/SemaCXX/copy-initialization.cpp
@@ -26,7 +26,7 @@
 };
 
 // PR3600
-void test(const foo *P) { P->bar(); } // expected-error{{'bar' not viable: 'this' argument has type 'const foo', but function is not marked const}}
+void test(const foo *P) { P->bar(); } // expected-error{{'this' argument to member function 'bar' has type 'const foo', but function is not marked const}}
 
 namespace PR6757 {
   struct Foo {
Index: test/CXX/over/over.match/over.match.funcs/p4-0x.cpp
===
--- test/CXX/over/over.match/over.match.funcs/p4-0x.cpp
+++ test/CXX/over/over.match/over.match.funcs/p4-0x.cpp
@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
-// expected-no-diagnostics
 
 template T ();
 template T &();
@@ -20,6 +19,18 @@
 
   void g();
 
+  void c() const; // expected-note {{'c' declared here}}
+  void v() volatile; // expected-note {{'v' declared here}}
+  void r() __restrict__; // expected-note {{'r' declared here}}
+  void cr() const __restrict__; // expected-note {{'cr' declared here}}
+  void cv() const volatile;
+  void vr() volatile __restrict__; // expected-note {{'vr' declared here}}
+  void cvr() const volatile __restrict__;
+
+  void lvalue() &; // expected-note 2 {{'lvalue' declared here}}
+  void const_lvalue() const&;
+  void rvalue() &&; // expected-note {{'rvalue' declared here}}
+
   int +(const X0&) &;
   float +(const X0&) &&;
 
@@ -32,7 +43,7 @@
   float () const&&;
 };
 
-void X0::g() {
+void X0::g() { // expected-note {{'g' declared here}}
   int  = f();
   int  = X0::f();
 }
@@ -69,3 +80,26 @@
   float  = xvalue().h2();
   float  = prvalue().h2();
 }
+
+void test_diagnostics(const volatile X0 &__restrict__ cvr) {
+  cvr.g(); // expected-error {{'this' argument to member function 'g' has type 'const volatile X0', but function is not marked const or volatile}}
+  cvr.c(); // expected-error {{not marked volatile}}
+  cvr.v(); // expected-error {{not marked const}}
+  cvr.r(); // expected-error {{not marked const or volatile}}
+  cvr.cr(); // expected-error {{not marked volatile}}
+  cvr.cv();
+  cvr.vr(); // expected-error {{not marked const}}
+  cvr.cvr();
+
+  lvalue().lvalue();
+  lvalue().const_lvalue();
+  lvalue().rvalue(); // expected-error {{'this' argument to member function 'rvalue' is an lvalue, but function has rvalue 

[PATCH] D40284: [Sema] Improve diagnostics for template arg deduction

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added a comment.

Bump :)


https://reviews.llvm.org/D40284



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

[PATCH] D39937: [Sema] Improve diagnostics for const- and ref-qualified member functions

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added a comment.

Bump :)


https://reviews.llvm.org/D39937



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

[PATCH] D40284: [Sema] Improve diagnostics for template arg deduction

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes updated this revision to Diff 124033.
jtbandes added a comment.

@erik.pilkington Updated to use a wrapper function. This is definitely less 
invasive, but it could defeat some optimizations (any approach that checks the 
return value will defeat tail recursion, I suppose...)


https://reviews.llvm.org/D40284

Files:
  lib/Sema/SemaTemplateDeduction.cpp
  test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/basic.cpp


Index: test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/basic.cpp
===
--- test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/basic.cpp
+++ test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/basic.cpp
@@ -45,3 +45,11 @@
 func(foo()); // expected-error {{no matching function}}
   }
 }
+
+namespace test4 {
+  // expected-note@+1 {{candidate template ignored: could not match 'int [N]' 
against 'int []'}}
+  template void f1(int ()[N]);
+  template void f2(int ()[]) {
+f1(arr); // expected-error {{no matching function}}
+  }
+}
Index: lib/Sema/SemaTemplateDeduction.cpp
===
--- lib/Sema/SemaTemplateDeduction.cpp
+++ lib/Sema/SemaTemplateDeduction.cpp
@@ -1056,6 +1056,12 @@
   return false;
 }
 
+static Sema::TemplateDeductionResult DeduceTemplateArgumentsByTypeMatchInner(
+Sema , TemplateParameterList *TemplateParams, QualType ParamIn,
+QualType ArgIn, TemplateDeductionInfo ,
+SmallVectorImpl , unsigned TDF,
+bool PartialOrdering, bool DeducedFromArrayBound);
+
 /// \brief Deduce the template arguments by comparing the parameter type and
 /// the argument type (C++ [temp.deduct.type]).
 ///
@@ -1080,15 +1086,34 @@
 /// \returns the result of template argument deduction so far. Note that a
 /// "success" result means that template argument deduction has not yet failed,
 /// but it may still fail, later, for other reasons.
-static Sema::TemplateDeductionResult
-DeduceTemplateArgumentsByTypeMatch(Sema ,
-   TemplateParameterList *TemplateParams,
-   QualType ParamIn, QualType ArgIn,
-   TemplateDeductionInfo ,
-SmallVectorImpl ,
-   unsigned TDF,
-   bool PartialOrdering,
-   bool DeducedFromArrayBound) {
+static Sema::TemplateDeductionResult DeduceTemplateArgumentsByTypeMatch(
+Sema , TemplateParameterList *TemplateParams, QualType ParamIn,
+QualType ArgIn, TemplateDeductionInfo ,
+SmallVectorImpl , unsigned TDF,
+bool PartialOrdering, bool DeducedFromArrayBound) {
+  // Because DeduceTemplateArgumentsByTypeMatchInner is recursive, and tends to
+  // modify Info.FirstArg and SecondArg even when deduction succeeds, save the
+  // original values and restore them if no error occurred.
+  const auto OriginalFirstArg = Info.FirstArg;
+  const auto OriginalSecondArg = Info.SecondArg;
+
+  const auto Result = DeduceTemplateArgumentsByTypeMatchInner(
+  S, TemplateParams, ParamIn, ArgIn, Info, Deduced, TDF, PartialOrdering,
+  DeducedFromArrayBound);
+
+  if (Result == Sema::TDK_Success) {
+Info.FirstArg = OriginalFirstArg;
+Info.SecondArg = OriginalSecondArg;
+  }
+  return Result;
+}
+
+/// \see DeduceTemplateArgumentsByTypeMatch()
+static Sema::TemplateDeductionResult DeduceTemplateArgumentsByTypeMatchInner(
+Sema , TemplateParameterList *TemplateParams, QualType ParamIn,
+QualType ArgIn, TemplateDeductionInfo ,
+SmallVectorImpl , unsigned TDF,
+bool PartialOrdering, bool DeducedFromArrayBound) {
   // We only want to look at the canonical types, since typedefs and
   // sugar are not part of template argument deduction.
   QualType Param = S.Context.getCanonicalType(ParamIn);


Index: test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/basic.cpp
===
--- test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/basic.cpp
+++ test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/basic.cpp
@@ -45,3 +45,11 @@
 func(foo()); // expected-error {{no matching function}}
   }
 }
+
+namespace test4 {
+  // expected-note@+1 {{candidate template ignored: could not match 'int [N]' against 'int []'}}
+  template void f1(int ()[N]);
+  template void f2(int ()[]) {
+f1(arr); // expected-error {{no matching function}}
+  }
+}
Index: lib/Sema/SemaTemplateDeduction.cpp
===
--- lib/Sema/SemaTemplateDeduction.cpp
+++ lib/Sema/SemaTemplateDeduction.cpp
@@ -1056,6 +1056,12 @@
   return false;
 }
 
+static Sema::TemplateDeductionResult DeduceTemplateArgumentsByTypeMatchInner(
+Sema , TemplateParameterList *TemplateParams, QualType ParamIn,
+QualType ArgIn, TemplateDeductionInfo ,
+SmallVectorImpl , unsigned TDF,
+bool 

[PATCH] D40284: [Sema] Improve diagnostics for template arg deduction

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added a reviewer: rsmith.
jtbandes added a subscriber: rsmith.
jtbandes added a comment.

Adding @rsmith for review based on the commit history of 
SemaTemplateDeduction.cpp :)


https://reviews.llvm.org/D40284



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

[PATCH] D39937: [Sema] Improve diagnostics for const- and ref-qualified member functions

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added a comment.

Thanks, will do. Is there an automated system that can run all the tests 
//before// I merge rather than waiting for a potential build failure after the 
fact?


https://reviews.llvm.org/D39937



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

[PATCH] D39937: [Sema] Improve diagnostics for const- and ref-qualified member functions

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes updated this revision to Diff 123481.
jtbandes added a comment.

- spell out full diagnostic the first time


https://reviews.llvm.org/D39937

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaOverload.cpp
  test/CXX/over/over.match/over.match.funcs/p4-0x.cpp
  test/SemaCXX/copy-initialization.cpp

Index: test/SemaCXX/copy-initialization.cpp
===
--- test/SemaCXX/copy-initialization.cpp
+++ test/SemaCXX/copy-initialization.cpp
@@ -26,7 +26,7 @@
 };
 
 // PR3600
-void test(const foo *P) { P->bar(); } // expected-error{{'bar' not viable: 'this' argument has type 'const foo', but function is not marked const}}
+void test(const foo *P) { P->bar(); } // expected-error{{'this' argument to member function 'bar' has type 'const foo', but function is not marked const}}
 
 namespace PR6757 {
   struct Foo {
Index: test/CXX/over/over.match/over.match.funcs/p4-0x.cpp
===
--- test/CXX/over/over.match/over.match.funcs/p4-0x.cpp
+++ test/CXX/over/over.match/over.match.funcs/p4-0x.cpp
@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
-// expected-no-diagnostics
 
 template T ();
 template T &();
@@ -20,6 +19,18 @@
 
   void g();
 
+  void c() const; // expected-note {{'c' declared here}}
+  void v() volatile; // expected-note {{'v' declared here}}
+  void r() __restrict__; // expected-note {{'r' declared here}}
+  void cr() const __restrict__; // expected-note {{'cr' declared here}}
+  void cv() const volatile;
+  void vr() volatile __restrict__; // expected-note {{'vr' declared here}}
+  void cvr() const volatile __restrict__;
+
+  void lvalue() &; // expected-note 2 {{'lvalue' declared here}}
+  void const_lvalue() const&;
+  void rvalue() &&; // expected-note {{'rvalue' declared here}}
+
   int +(const X0&) &;
   float +(const X0&) &&;
 
@@ -32,7 +43,7 @@
   float () const&&;
 };
 
-void X0::g() {
+void X0::g() { // expected-note {{'g' declared here}}
   int  = f();
   int  = X0::f();
 }
@@ -69,3 +80,26 @@
   float  = xvalue().h2();
   float  = prvalue().h2();
 }
+
+void test_diagnostics(const volatile X0 &__restrict__ cvr) {
+  cvr.g(); // expected-error {{'this' argument to member function 'g' has type 'const volatile X0', but function is not marked const or volatile}}
+  cvr.c(); // expected-error {{not marked volatile}}
+  cvr.v(); // expected-error {{not marked const}}
+  cvr.r(); // expected-error {{not marked const or volatile}}
+  cvr.cr(); // expected-error {{not marked volatile}}
+  cvr.cv();
+  cvr.vr(); // expected-error {{not marked const}}
+  cvr.cvr();
+
+  lvalue().lvalue();
+  lvalue().const_lvalue();
+  lvalue().rvalue(); // expected-error {{'this' argument to member function 'rvalue' is an lvalue, but function has rvalue ref-qualifier}}
+
+  xvalue().lvalue(); // expected-error {{'this' argument to member function 'lvalue' is an rvalue, but function has non-const lvalue ref-qualifier}}
+  xvalue().const_lvalue();
+  xvalue().rvalue();
+
+  prvalue().lvalue(); // expected-error {{'this' argument to member function 'lvalue' is an rvalue, but function has non-const lvalue ref-qualifier}}
+  prvalue().const_lvalue();
+  prvalue().rvalue();
+}
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -5145,7 +5145,8 @@
   *this, From->getLocStart(), From->getType(), FromClassification, Method,
   Method->getParent());
   if (ICS.isBad()) {
-if (ICS.Bad.Kind == BadConversionSequence::bad_qualifiers) {
+switch (ICS.Bad.Kind) {
+case BadConversionSequence::bad_qualifiers: {
   Qualifiers FromQs = FromRecordType.getQualifiers();
   Qualifiers ToQs = DestType.getQualifiers();
   unsigned CVR = FromQs.getCVRQualifiers() & ~ToQs.getCVRQualifiers();
@@ -5158,10 +5159,28 @@
   << Method->getDeclName();
 return ExprError();
   }
+  break;
+}
+
+case BadConversionSequence::lvalue_ref_to_rvalue:
+case BadConversionSequence::rvalue_ref_to_lvalue: {
+  bool IsRValueQualified =
+Method->getRefQualifier() == RefQualifierKind::RQ_RValue;
+  Diag(From->getLocStart(), diag::err_member_function_call_bad_ref)
+<< Method->getDeclName() << FromClassification.isRValue()
+<< IsRValueQualified;
+  Diag(Method->getLocation(), diag::note_previous_decl)
+<< Method->getDeclName();
+  return ExprError();
+}
+
+case BadConversionSequence::no_conversion:
+case BadConversionSequence::unrelated_class:
+  break;
 }
 
 return Diag(From->getLocStart(),
-diag::err_implicit_object_parameter_init)
+diag::err_member_function_call_bad_type)
<< ImplicitParamRecordType << FromRecordType << From->getSourceRange();
   }
 
Index: include/clang/Basic/DiagnosticSemaKinds.td

[PATCH] D39937: [Sema] Improve diagnostics for const- and ref-qualified member functions

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes updated this revision to Diff 123476.
jtbandes added a comment.

- feedback from review & more tests


https://reviews.llvm.org/D39937

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaOverload.cpp
  test/CXX/over/over.match/over.match.funcs/p4-0x.cpp
  test/SemaCXX/copy-initialization.cpp

Index: test/SemaCXX/copy-initialization.cpp
===
--- test/SemaCXX/copy-initialization.cpp
+++ test/SemaCXX/copy-initialization.cpp
@@ -26,7 +26,7 @@
 };
 
 // PR3600
-void test(const foo *P) { P->bar(); } // expected-error{{'bar' not viable: 'this' argument has type 'const foo', but function is not marked const}}
+void test(const foo *P) { P->bar(); } // expected-error{{'this' argument to member function 'bar' has type 'const foo', but function is not marked const}}
 
 namespace PR6757 {
   struct Foo {
Index: test/CXX/over/over.match/over.match.funcs/p4-0x.cpp
===
--- test/CXX/over/over.match/over.match.funcs/p4-0x.cpp
+++ test/CXX/over/over.match/over.match.funcs/p4-0x.cpp
@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
-// expected-no-diagnostics
 
 template T ();
 template T &();
@@ -20,6 +19,18 @@
 
   void g();
 
+  void c() const; // expected-note {{'c' declared here}}
+  void v() volatile; // expected-note {{'v' declared here}}
+  void r() __restrict__; // expected-note {{'r' declared here}}
+  void cr() const __restrict__; // expected-note {{'cr' declared here}}
+  void cv() const volatile;
+  void vr() volatile __restrict__; // expected-note {{'vr' declared here}}
+  void cvr() const volatile __restrict__;
+
+  void lvalue() &; // expected-note 2 {{'lvalue' declared here}}
+  void const_lvalue() const&;
+  void rvalue() &&; // expected-note {{'rvalue' declared here}}
+
   int +(const X0&) &;
   float +(const X0&) &&;
 
@@ -32,7 +43,7 @@
   float () const&&;
 };
 
-void X0::g() {
+void X0::g() { // expected-note {{'g' declared here}}
   int  = f();
   int  = X0::f();
 }
@@ -69,3 +80,26 @@
   float  = xvalue().h2();
   float  = prvalue().h2();
 }
+
+void test_diagnostics(const volatile X0 &__restrict__ cvr) {
+  cvr.g(); // expected-error {{not marked const or volatile}}
+  cvr.c(); // expected-error {{not marked volatile}}
+  cvr.v(); // expected-error {{not marked const}}
+  cvr.r(); // expected-error {{not marked const or volatile}}
+  cvr.cr(); // expected-error {{not marked volatile}}
+  cvr.cv();
+  cvr.vr(); // expected-error {{not marked const}}
+  cvr.cvr();
+
+  lvalue().lvalue();
+  lvalue().const_lvalue();
+  lvalue().rvalue(); // expected-error {{'this' argument to member function 'rvalue' is an lvalue, but function has rvalue ref-qualifier}}
+
+  xvalue().lvalue(); // expected-error {{'this' argument to member function 'lvalue' is an rvalue, but function has non-const lvalue ref-qualifier}}
+  xvalue().const_lvalue();
+  xvalue().rvalue();
+
+  prvalue().lvalue(); // expected-error {{'this' argument to member function 'lvalue' is an rvalue, but function has non-const lvalue ref-qualifier}}
+  prvalue().const_lvalue();
+  prvalue().rvalue();
+}
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -5145,7 +5145,8 @@
   *this, From->getLocStart(), From->getType(), FromClassification, Method,
   Method->getParent());
   if (ICS.isBad()) {
-if (ICS.Bad.Kind == BadConversionSequence::bad_qualifiers) {
+switch (ICS.Bad.Kind) {
+case BadConversionSequence::bad_qualifiers: {
   Qualifiers FromQs = FromRecordType.getQualifiers();
   Qualifiers ToQs = DestType.getQualifiers();
   unsigned CVR = FromQs.getCVRQualifiers() & ~ToQs.getCVRQualifiers();
@@ -5158,10 +5159,28 @@
   << Method->getDeclName();
 return ExprError();
   }
+  break;
+}
+
+case BadConversionSequence::lvalue_ref_to_rvalue:
+case BadConversionSequence::rvalue_ref_to_lvalue: {
+  bool IsRValueQualified =
+Method->getRefQualifier() == RefQualifierKind::RQ_RValue;
+  Diag(From->getLocStart(), diag::err_member_function_call_bad_ref)
+<< Method->getDeclName() << FromClassification.isRValue()
+<< IsRValueQualified;
+  Diag(Method->getLocation(), diag::note_previous_decl)
+<< Method->getDeclName();
+  return ExprError();
+}
+
+case BadConversionSequence::no_conversion:
+case BadConversionSequence::unrelated_class:
+  break;
 }
 
 return Diag(From->getLocStart(),
-diag::err_implicit_object_parameter_init)
+diag::err_member_function_call_bad_type)
<< ImplicitParamRecordType << FromRecordType << From->getSourceRange();
   }
 
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ 

[PATCH] D39937: [Sema] Improve diagnostics for const- and ref-qualified member functions

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added inline comments.



Comment at: test/CXX/over/over.match/over.match.funcs/p4-0x.cpp:22-24
+  void lvalue() &; // expected-note 2 {{'lvalue' declared here}}
+  void const_lvalue() const&;
+  void rvalue() &&; // expected-note {{'rvalue' declared here}}

aaron.ballman wrote:
> Can you add examples that cover the other diagnostic wordings as well 
> (volatile, restrict, combinations, etc)?
I've been working on this, but I actually can't trigger the `restrict` 
variants. Do you know whether this is something that's expected to work? The 
implicit object param doesn't seem to retain its restrict-ness (full 
disclosure, I have almost no prior experience with `restrict`...):

```
  void c() const;
  void v() volatile;
  void r() __restrict__;
  void cr() const __restrict__;
  void cv() const volatile;
  void vr() volatile __restrict__;
  void cvr() const volatile __restrict__;
```
```
void test_diagnostics(const volatile X0 &__restrict__ cvr) {
  cvr.g(); // expected-error {{not marked const, volatile, or restrict}}  -- 
actually produces "not marked const or volatile"
  cvr.c(); // expected-error {{not marked volatile or restrict}}  -- actually 
produces "not marked volatile"
  cvr.v(); // expected-error {{not marked const or restrict}}  -- actually 
produces "not marked const"
  cvr.r(); // expected-error {{not marked const or volatile}}
  cvr.cr(); // expected-error {{not marked volatile}}
  cvr.cv(); // expected-error {{not marked restrict}}  -- actually produces no 
error
  cvr.vr(); // expected-error {{not marked const}}
  cvr.cvr();
}
```


https://reviews.llvm.org/D39937



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

[PATCH] D39937: [Sema] Improve diagnostics for const- and ref-qualified member functions

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:1595-1597
+def err_member_function_call_other : Error<
+  "cannot initialize object parameter of type %0 with an expression "
+  "of type %1">;

aaron.ballman wrote:
> I don't think this diagnostic needs to be moved and renamed; the old name was 
> more clear than "other" on the new name.
How about `_bad_type`? I was hoping to give these all similar names since they 
happen during the same implicit conversion. Also, while "implicit object 
parameter" is technically a correct name, it seems to be not very commonly used 
and would confuse users. Though it's also valuable for the diagnostic id to be 
similar to the actual text...


https://reviews.llvm.org/D39937



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

[PATCH] D39937: [Sema] Improve diagnostics for const- and ref-qualified member functions

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes created this revision.

Adjust wording for const-qualification mismatch to be a little more clear.

Also add another diagnostic for a ref qualifier mismatch, which previously 
produced a useless error (this error path is simply very old; see 
https://reviews.llvm.org/rL119336):

Before:

  error: cannot initialize object parameter of type 'X0' with an expression of 
type 'X0'

After:

  error: 'this' argument to member function 'rvalue' is an lvalue, but function 
has rvalue ref-qualifier


https://reviews.llvm.org/D39937

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaOverload.cpp
  test/CXX/over/over.match/over.match.funcs/p4-0x.cpp
  test/SemaCXX/copy-initialization.cpp

Index: test/SemaCXX/copy-initialization.cpp
===
--- test/SemaCXX/copy-initialization.cpp
+++ test/SemaCXX/copy-initialization.cpp
@@ -26,7 +26,7 @@
 };
 
 // PR3600
-void test(const foo *P) { P->bar(); } // expected-error{{'bar' not viable: 'this' argument has type 'const foo', but function is not marked const}}
+void test(const foo *P) { P->bar(); } // expected-error{{'this' argument to member function 'bar' has type 'const foo', but function is not marked const}}
 
 namespace PR6757 {
   struct Foo {
Index: test/CXX/over/over.match/over.match.funcs/p4-0x.cpp
===
--- test/CXX/over/over.match/over.match.funcs/p4-0x.cpp
+++ test/CXX/over/over.match/over.match.funcs/p4-0x.cpp
@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
-// expected-no-diagnostics
 
 template T ();
 template T &();
@@ -20,6 +19,10 @@
 
   void g();
 
+  void lvalue() &; // expected-note 2 {{'lvalue' declared here}}
+  void const_lvalue() const&;
+  void rvalue() &&; // expected-note {{'rvalue' declared here}}
+
   int +(const X0&) &;
   float +(const X0&) &&;
 
@@ -44,6 +47,18 @@
   int  = lvalue().ft(1);
   float  = xvalue().ft(2);
   float  = prvalue().ft(3);
+
+  lvalue().lvalue();
+  lvalue().const_lvalue();
+  lvalue().rvalue(); // expected-error {{'this' argument to member function 'rvalue' is an lvalue, but function has rvalue ref-qualifier}}
+
+  xvalue().lvalue(); // expected-error {{'this' argument to member function 'lvalue' is an rvalue, but function has non-const lvalue ref-qualifier}}
+  xvalue().const_lvalue();
+  xvalue().rvalue();
+
+  prvalue().lvalue(); // expected-error {{'this' argument to member function 'lvalue' is an rvalue, but function has non-const lvalue ref-qualifier}}
+  prvalue().const_lvalue();
+  prvalue().rvalue();
 }
 
 void test_ref_qualifier_binding_with_surrogates() {
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -5145,7 +5145,8 @@
   *this, From->getLocStart(), From->getType(), FromClassification, Method,
   Method->getParent());
   if (ICS.isBad()) {
-if (ICS.Bad.Kind == BadConversionSequence::bad_qualifiers) {
+switch (ICS.Bad.Kind) {
+case BadConversionSequence::bad_qualifiers: {
   Qualifiers FromQs = FromRecordType.getQualifiers();
   Qualifiers ToQs = DestType.getQualifiers();
   unsigned CVR = FromQs.getCVRQualifiers() & ~ToQs.getCVRQualifiers();
@@ -5158,10 +5159,27 @@
   << Method->getDeclName();
 return ExprError();
   }
+  break;
+}
+
+case BadConversionSequence::lvalue_ref_to_rvalue:
+case BadConversionSequence::rvalue_ref_to_lvalue: {
+  bool IsRValueQualified =
+Method->getRefQualifier() == RefQualifierKind::RQ_RValue;
+  Diag(From->getLocStart(), diag::err_member_function_call_bad_ref)
+<< Method->getDeclName() << FromClassification.isRValue()
+<< IsRValueQualified;
+  Diag(Method->getLocation(), diag::note_previous_decl)
+<< Method->getDeclName();
+  return ExprError();
+}
+
+default:
+  break;
 }
 
 return Diag(From->getLocStart(),
-diag::err_implicit_object_parameter_init)
+diag::err_member_function_call_other)
<< ImplicitParamRecordType << FromRecordType << From->getSourceRange();
   }
 
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -1582,12 +1582,20 @@
 def ext_pure_function_definition : ExtWarn<
   "function definition with pure-specifier is a Microsoft extension">,
   InGroup;
-def err_implicit_object_parameter_init : Error<
-  "cannot initialize object parameter of type %0 with an expression "
-  "of type %1">;
 def err_qualified_member_of_unrelated : Error<
   "%q0 is not a member of class %1">;
 
+def err_member_function_call_bad_ref : Error<
+  "'this' argument to member function %0 is an %select{lvalue|rvalue}1, "
+  "but function has %select{non-const lvalue|rvalue}2 

[PATCH] D29660: [OpenMP] Add flag for overwriting default PTX version for OpenMP targets

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added a comment.

@gtbercea Hi, I just saw your comment on my gist. (Unfortunately github does 
not send email notifications about gist comments; commenting here is probably 
better.) If you have Docker installed, it should be easy to get whatever output 
you like — just change the Dockerfile to use `-DCMAKE_BUILD_TYPE=Debug`, then 
run `docker build -t llvm-test .` and `docker run -it llvm-test /bin/bash`.


Repository:
  rL LLVM

https://reviews.llvm.org/D29660



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

[PATCH] D29660: [OpenMP] Add flag for overwriting default PTX version for OpenMP targets

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added a comment.

I'm still seeing a failure after r301549: 
https://gist.github.com/jtbandes/de6118abaadc6c5a5c9b4223a62f596c


Repository:
  rL LLVM

https://reviews.llvm.org/D29660



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

[PATCH] D29660: [OpenMP] Add flag for overwriting default PTX version for OpenMP targets

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added a comment.

FWIW, I'm able to reproduce the failure using Docker:

Dockerfile:

  FROM ubuntu:xenial
  RUN apt-get update
  RUN apt-get install -y build-essential ca-certificates subversion python 
cmake --no-install-recommends
  
  WORKDIR /
  RUN svn co -q -r 310537 http://llvm.org/svn/llvm-project/llvm/trunk llvm
  RUN svn co -q -r 310537 http://llvm.org/svn/llvm-project/cfe/trunk 
llvm/tools/clang
  
  RUN mkdir /build
  WORKDIR /build
  
  RUN cmake ../llvm -DCMAKE_BUILD_TYPE="Release"



  $ docker build -t D29660-test . && docker run -it D29660-test /bin/bash

then inside the container: `make check-clang-driver -j8`


Repository:
  rL LLVM

https://reviews.llvm.org/D29660



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

[PATCH] D36019: [clang-format] Fix bug with ENAS_DontAlign and empty lines

[Jacob Bandes-Storch via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL310539: clang-format: Fix bug with ENAS_DontAlign and empty 
lines (authored by jtbandes).

Repository:
  rL LLVM

https://reviews.llvm.org/D36019

Files:
  cfe/trunk/lib/Format/WhitespaceManager.cpp
  cfe/trunk/lib/Format/WhitespaceManager.h
  cfe/trunk/unittests/Format/FormatTest.cpp


Index: cfe/trunk/lib/Format/WhitespaceManager.h
===
--- cfe/trunk/lib/Format/WhitespaceManager.h
+++ cfe/trunk/lib/Format/WhitespaceManager.h
@@ -195,9 +195,9 @@
   /// \brief Stores \p Text as the replacement for the whitespace in \p Range.
   void storeReplacement(SourceRange Range, StringRef Text);
   void appendNewlineText(std::string , unsigned Newlines);
-  void appendNewlineText(std::string , unsigned Newlines,
- unsigned PreviousEndOfTokenColumn,
- unsigned EscapedNewlineColumn);
+  void appendEscapedNewlineText(std::string , unsigned Newlines,
+unsigned PreviousEndOfTokenColumn,
+unsigned EscapedNewlineColumn);
   void appendIndentText(std::string , unsigned IndentLevel,
 unsigned Spaces, unsigned WhitespaceStartColumn);
 
Index: cfe/trunk/lib/Format/WhitespaceManager.cpp
===
--- cfe/trunk/lib/Format/WhitespaceManager.cpp
+++ cfe/trunk/lib/Format/WhitespaceManager.cpp
@@ -603,8 +603,9 @@
 if (C.CreateReplacement) {
   std::string ReplacementText = C.PreviousLinePostfix;
   if (C.ContinuesPPDirective)
-appendNewlineText(ReplacementText, C.NewlinesBefore,
-  C.PreviousEndOfTokenColumn, C.EscapedNewlineColumn);
+appendEscapedNewlineText(ReplacementText, C.NewlinesBefore,
+ C.PreviousEndOfTokenColumn,
+ C.EscapedNewlineColumn);
   else
 appendNewlineText(ReplacementText, C.NewlinesBefore);
   appendIndentText(ReplacementText, C.Tok->IndentLevel,
@@ -640,16 +641,17 @@
 Text.append(UseCRLF ? "\r\n" : "\n");
 }
 
-void WhitespaceManager::appendNewlineText(std::string , unsigned Newlines,
-  unsigned PreviousEndOfTokenColumn,
-  unsigned EscapedNewlineColumn) {
+void WhitespaceManager::appendEscapedNewlineText(std::string ,
+ unsigned Newlines,
+ unsigned 
PreviousEndOfTokenColumn,
+ unsigned 
EscapedNewlineColumn) {
   if (Newlines > 0) {
-unsigned Offset =
-std::min(EscapedNewlineColumn - 2, PreviousEndOfTokenColumn);
+unsigned Spaces =
+std::max(1, EscapedNewlineColumn - PreviousEndOfTokenColumn - 1);
 for (unsigned i = 0; i < Newlines; ++i) {
-  Text.append(EscapedNewlineColumn - Offset - 1, ' ');
+  Text.append(Spaces, ' ');
   Text.append(UseCRLF ? "\\\r\n" : "\\\n");
-  Offset = 0;
+  Spaces = std::max(0, EscapedNewlineColumn - 1);
 }
   }
 }
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -2309,6 +2309,30 @@
   EXPECT_EQ("template  f();", format("\\\ntemplate  f();"));
   EXPECT_EQ("/* \\  \\  \\\n */", format("\\\n/* \\  \\  \\\n */"));
   EXPECT_EQ("", format(""));
+
+  FormatStyle DontAlign = getLLVMStyle();
+  DontAlign.AlignEscapedNewlines = FormatStyle::ENAS_DontAlign;
+  DontAlign.MaxEmptyLinesToKeep = 3;
+  // FIXME: can't use verifyFormat here because the newline before
+  // "public:" is not inserted the first time it's reformatted
+  EXPECT_EQ("#define A \\\n"
+"  class Foo { \\\n"
+"void bar(); \\\n"
+"\\\n"
+"\\\n"
+"\\\n"
+"  public: \\\n"
+"void baz(); \\\n"
+"  };",
+format("#define A \\\n"
+   "  class Foo { \\\n"
+   "void bar(); \\\n"
+   "\\\n"
+   "\\\n"
+   "\\\n"
+   "  public: \\\n"
+   "void baz(); \\\n"
+   "  };", DontAlign));
 }
 
 TEST_F(FormatTest, CalculateSpaceOnConsecutiveLinesInMacro) {


Index: cfe/trunk/lib/Format/WhitespaceManager.h
===
--- cfe/trunk/lib/Format/WhitespaceManager.h
+++ cfe/trunk/lib/Format/WhitespaceManager.h
@@ -195,9 +195,9 @@
   /// \brief Stores \p Text as the replacement for the whitespace in \p Range.
   void storeReplacement(SourceRange Range, StringRef Text);
   void appendNewlineText(std::string , unsigned 

[PATCH] D34324: [clang-format] let PointerAlignment dictate spacing of function ref qualifiers

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added a comment.

@djasper bump, any thoughts on this?


https://reviews.llvm.org/D34324



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

[PATCH] D36019: [clang-format] Fix bug with ENAS_DontAlign and empty lines

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added a comment.

Thanks. Can you commit this when you get a chance? I don't have permissions.


https://reviews.llvm.org/D36019



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

[PATCH] D36019: [clang-format] Fix bug with ENAS_DontAlign and empty lines

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added inline comments.



Comment at: lib/Format/WhitespaceManager.cpp:650
+for (unsigned i = 0; i < Newlines; ++i)
+  Text.append(UseCRLF ? " \\\r\n" : " \\\n");
+return;

djasper wrote:
> Note that when you have an empty line, this would turn into:
> 
>   #define A \
> int i; \
>\<-- Note the 1-space indent here.
> int j; \
> int k;
> 
> With my alternative below, that "\" will just be put at column 0, which 
> probably isn't better or worse.
I suppose that can be changed back to 1 by using `std::max(1, 
EscapedNewlineColumn - 1);` instead, right? I don't have strong feelings about 
whether it should be 0 or 1.


https://reviews.llvm.org/D36019



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

[PATCH] D36019: [clang-format] Fix bug with ENAS_DontAlign and empty lines

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes updated this revision to Diff 109898.
jtbandes added a comment.

@djasper ok, done


https://reviews.llvm.org/D36019

Files:
  lib/Format/WhitespaceManager.cpp
  lib/Format/WhitespaceManager.h
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2309,6 +2309,30 @@
   EXPECT_EQ("template  f();", format("\\\ntemplate  f();"));
   EXPECT_EQ("/* \\  \\  \\\n */", format("\\\n/* \\  \\  \\\n */"));
   EXPECT_EQ("", format(""));
+
+  FormatStyle DontAlign = getLLVMStyle();
+  DontAlign.AlignEscapedNewlines = FormatStyle::ENAS_DontAlign;
+  DontAlign.MaxEmptyLinesToKeep = 3;
+  // FIXME: can't use verifyFormat here because the newline before
+  // "public:" is not inserted the first time it's reformatted
+  EXPECT_EQ("#define A \\\n"
+"  class Foo { \\\n"
+"void bar(); \\\n"
+"\\\n"
+"\\\n"
+"\\\n"
+"  public: \\\n"
+"void baz(); \\\n"
+"  };",
+format("#define A \\\n"
+   "  class Foo { \\\n"
+   "void bar(); \\\n"
+   "\\\n"
+   "\\\n"
+   "\\\n"
+   "  public: \\\n"
+   "void baz(); \\\n"
+   "  };", DontAlign));
 }
 
 TEST_F(FormatTest, CalculateSpaceOnConsecutiveLinesInMacro) {
Index: lib/Format/WhitespaceManager.h
===
--- lib/Format/WhitespaceManager.h
+++ lib/Format/WhitespaceManager.h
@@ -195,9 +195,9 @@
   /// \brief Stores \p Text as the replacement for the whitespace in \p Range.
   void storeReplacement(SourceRange Range, StringRef Text);
   void appendNewlineText(std::string , unsigned Newlines);
-  void appendNewlineText(std::string , unsigned Newlines,
- unsigned PreviousEndOfTokenColumn,
- unsigned EscapedNewlineColumn);
+  void appendEscapedNewlineText(std::string , unsigned Newlines,
+unsigned PreviousEndOfTokenColumn,
+unsigned EscapedNewlineColumn);
   void appendIndentText(std::string , unsigned IndentLevel,
 unsigned Spaces, unsigned WhitespaceStartColumn);
 
Index: lib/Format/WhitespaceManager.cpp
===
--- lib/Format/WhitespaceManager.cpp
+++ lib/Format/WhitespaceManager.cpp
@@ -603,8 +603,9 @@
 if (C.CreateReplacement) {
   std::string ReplacementText = C.PreviousLinePostfix;
   if (C.ContinuesPPDirective)
-appendNewlineText(ReplacementText, C.NewlinesBefore,
-  C.PreviousEndOfTokenColumn, C.EscapedNewlineColumn);
+appendEscapedNewlineText(ReplacementText, C.NewlinesBefore,
+ C.PreviousEndOfTokenColumn,
+ C.EscapedNewlineColumn);
   else
 appendNewlineText(ReplacementText, C.NewlinesBefore);
   appendIndentText(ReplacementText, C.Tok->IndentLevel,
@@ -640,16 +641,17 @@
 Text.append(UseCRLF ? "\r\n" : "\n");
 }
 
-void WhitespaceManager::appendNewlineText(std::string , unsigned Newlines,
-  unsigned PreviousEndOfTokenColumn,
-  unsigned EscapedNewlineColumn) {
+void WhitespaceManager::appendEscapedNewlineText(std::string ,
+ unsigned Newlines,
+ unsigned 
PreviousEndOfTokenColumn,
+ unsigned 
EscapedNewlineColumn) {
   if (Newlines > 0) {
-unsigned Offset =
-std::min(EscapedNewlineColumn - 2, PreviousEndOfTokenColumn);
+unsigned Spaces =
+std::max(1, EscapedNewlineColumn - PreviousEndOfTokenColumn - 1);
 for (unsigned i = 0; i < Newlines; ++i) {
-  Text.append(EscapedNewlineColumn - Offset - 1, ' ');
+  Text.append(Spaces, ' ');
   Text.append(UseCRLF ? "\\\r\n" : "\\\n");
-  Offset = 0;
+  Spaces = std::max(0, EscapedNewlineColumn - 1);
 }
   }
 }


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2309,6 +2309,30 @@
   EXPECT_EQ("template  f();", format("\\\ntemplate  f();"));
   EXPECT_EQ("/* \\  \\  \\\n */", format("\\\n/* \\  \\  \\\n */"));
   EXPECT_EQ("", format(""));
+
+  FormatStyle DontAlign = getLLVMStyle();
+  DontAlign.AlignEscapedNewlines = FormatStyle::ENAS_DontAlign;
+  DontAlign.MaxEmptyLinesToKeep = 3;
+  // FIXME: can't use verifyFormat here because the newline before
+  // "public:" is not inserted the first time 

[PATCH] D36019: [clang-format] Fix bug with ENAS_DontAlign and empty lines

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added a comment.

@djasper Bump :)


https://reviews.llvm.org/D36019



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

[PATCH] D36019: [clang-format] Fix bug with ENAS_DontAlign and empty lines

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes updated this revision to Diff 108863.
jtbandes added a comment.

- Undo change in argument list


https://reviews.llvm.org/D36019

Files:
  lib/Format/WhitespaceManager.cpp
  lib/Format/WhitespaceManager.h
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2309,6 +2309,30 @@
   EXPECT_EQ("template  f();", format("\\\ntemplate  f();"));
   EXPECT_EQ("/* \\  \\  \\\n */", format("\\\n/* \\  \\  \\\n */"));
   EXPECT_EQ("", format(""));
+
+  FormatStyle DontAlign = getLLVMStyle();
+  DontAlign.AlignEscapedNewlines = FormatStyle::ENAS_DontAlign;
+  DontAlign.MaxEmptyLinesToKeep = 3;
+  // FIXME: can't use verifyFormat here because the newline before
+  // "public:" is not inserted the first time it's reformatted
+  EXPECT_EQ("#define A \\\n"
+"  class Foo { \\\n"
+"void bar(); \\\n"
+" \\\n"
+" \\\n"
+" \\\n"
+"  public: \\\n"
+"void baz(); \\\n"
+"  };",
+format("#define A \\\n"
+   "  class Foo { \\\n"
+   "void bar(); \\\n"
+   " \\\n"
+   " \\\n"
+   " \\\n"
+   "  public: \\\n"
+   "void baz(); \\\n"
+   "  };", DontAlign));
 }
 
 TEST_F(FormatTest, CalculateSpaceOnConsecutiveLinesInMacro) {
Index: lib/Format/WhitespaceManager.h
===
--- lib/Format/WhitespaceManager.h
+++ lib/Format/WhitespaceManager.h
@@ -195,9 +195,9 @@
   /// \brief Stores \p Text as the replacement for the whitespace in \p Range.
   void storeReplacement(SourceRange Range, StringRef Text);
   void appendNewlineText(std::string , unsigned Newlines);
-  void appendNewlineText(std::string , unsigned Newlines,
- unsigned PreviousEndOfTokenColumn,
- unsigned EscapedNewlineColumn);
+  void appendEscapedNewlineText(std::string , unsigned Newlines,
+unsigned PreviousEndOfTokenColumn,
+unsigned EscapedNewlineColumn);
   void appendIndentText(std::string , unsigned IndentLevel,
 unsigned Spaces, unsigned WhitespaceStartColumn);
 
Index: lib/Format/WhitespaceManager.cpp
===
--- lib/Format/WhitespaceManager.cpp
+++ lib/Format/WhitespaceManager.cpp
@@ -603,8 +603,9 @@
 if (C.CreateReplacement) {
   std::string ReplacementText = C.PreviousLinePostfix;
   if (C.ContinuesPPDirective)
-appendNewlineText(ReplacementText, C.NewlinesBefore,
-  C.PreviousEndOfTokenColumn, C.EscapedNewlineColumn);
+appendEscapedNewlineText(ReplacementText, C.NewlinesBefore,
+ C.PreviousEndOfTokenColumn,
+ C.EscapedNewlineColumn);
   else
 appendNewlineText(ReplacementText, C.NewlinesBefore);
   appendIndentText(ReplacementText, C.Tok->IndentLevel,
@@ -640,12 +641,20 @@
 Text.append(UseCRLF ? "\r\n" : "\n");
 }
 
-void WhitespaceManager::appendNewlineText(std::string , unsigned Newlines,
-  unsigned PreviousEndOfTokenColumn,
-  unsigned EscapedNewlineColumn) {
+void WhitespaceManager::appendEscapedNewlineText(std::string ,
+ unsigned Newlines,
+ unsigned 
PreviousEndOfTokenColumn,
+ unsigned 
EscapedNewlineColumn) {
+  if (Style.AlignEscapedNewlines == FormatStyle::ENAS_DontAlign) {
+for (unsigned i = 0; i < Newlines; ++i)
+  Text.append(UseCRLF ? " \\\r\n" : " \\\n");
+return;
+  }
+
   if (Newlines > 0) {
+assert(EscapedNewlineColumn >= 1);
 unsigned Offset =
-std::min(EscapedNewlineColumn - 2, PreviousEndOfTokenColumn);
+std::min(EscapedNewlineColumn - 1, PreviousEndOfTokenColumn);
 for (unsigned i = 0; i < Newlines; ++i) {
   Text.append(EscapedNewlineColumn - Offset - 1, ' ');
   Text.append(UseCRLF ? "\\\r\n" : "\\\n");


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2309,6 +2309,30 @@
   EXPECT_EQ("template  f();", format("\\\ntemplate  f();"));
   EXPECT_EQ("/* \\  \\  \\\n */", format("\\\n/* \\  \\  \\\n */"));
   EXPECT_EQ("", format(""));
+
+  FormatStyle DontAlign = getLLVMStyle();
+  DontAlign.AlignEscapedNewlines = FormatStyle::ENAS_DontAlign;
+  DontAlign.MaxEmptyLinesToKeep = 3;
+  // FIXME: can't 

[PATCH] D36019: [clang-format] Fix bug with ENAS_DontAlign and empty lines

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes updated this revision to Diff 108860.
jtbandes added a comment.

Okay, I think this approach is better:

- Rename the version of `appendNewlineText` used for escaped newlines to 
`appendEscapedNewlineText` to reduce confusability.
- If `ENAS_DontAlign`, skip all of the offset calculation logic. Just append 
space-backslash-newline.
- Restore the offset calculation to use `EscapedNewlineColumn - 1`, which it 
was before https://reviews.llvm.org/D32733. I don't think there was a good 
reason to change this.
- Leave in the `assert`.


https://reviews.llvm.org/D36019

Files:
  lib/Format/WhitespaceManager.cpp
  lib/Format/WhitespaceManager.h
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2309,6 +2309,30 @@
   EXPECT_EQ("template  f();", format("\\\ntemplate  f();"));
   EXPECT_EQ("/* \\  \\  \\\n */", format("\\\n/* \\  \\  \\\n */"));
   EXPECT_EQ("", format(""));
+
+  FormatStyle DontAlign = getLLVMStyle();
+  DontAlign.AlignEscapedNewlines = FormatStyle::ENAS_DontAlign;
+  DontAlign.MaxEmptyLinesToKeep = 3;
+  // FIXME: can't use verifyFormat here because the newline before
+  // "public:" is not inserted the first time it's reformatted
+  EXPECT_EQ("#define A \\\n"
+"  class Foo { \\\n"
+"void bar(); \\\n"
+" \\\n"
+" \\\n"
+" \\\n"
+"  public: \\\n"
+"void baz(); \\\n"
+"  };",
+format("#define A \\\n"
+   "  class Foo { \\\n"
+   "void bar(); \\\n"
+   " \\\n"
+   " \\\n"
+   " \\\n"
+   "  public: \\\n"
+   "void baz(); \\\n"
+   "  };", DontAlign));
 }
 
 TEST_F(FormatTest, CalculateSpaceOnConsecutiveLinesInMacro) {
Index: lib/Format/WhitespaceManager.h
===
--- lib/Format/WhitespaceManager.h
+++ lib/Format/WhitespaceManager.h
@@ -195,9 +195,7 @@
   /// \brief Stores \p Text as the replacement for the whitespace in \p Range.
   void storeReplacement(SourceRange Range, StringRef Text);
   void appendNewlineText(std::string , unsigned Newlines);
-  void appendNewlineText(std::string , unsigned Newlines,
- unsigned PreviousEndOfTokenColumn,
- unsigned EscapedNewlineColumn);
+  void appendEscapedNewlineText(std::string , const Change );
   void appendIndentText(std::string , unsigned IndentLevel,
 unsigned Spaces, unsigned WhitespaceStartColumn);
 
Index: lib/Format/WhitespaceManager.cpp
===
--- lib/Format/WhitespaceManager.cpp
+++ lib/Format/WhitespaceManager.cpp
@@ -603,8 +603,7 @@
 if (C.CreateReplacement) {
   std::string ReplacementText = C.PreviousLinePostfix;
   if (C.ContinuesPPDirective)
-appendNewlineText(ReplacementText, C.NewlinesBefore,
-  C.PreviousEndOfTokenColumn, C.EscapedNewlineColumn);
+appendEscapedNewlineText(ReplacementText, C);
   else
 appendNewlineText(ReplacementText, C.NewlinesBefore);
   appendIndentText(ReplacementText, C.Tok->IndentLevel,
@@ -640,14 +639,19 @@
 Text.append(UseCRLF ? "\r\n" : "\n");
 }
 
-void WhitespaceManager::appendNewlineText(std::string , unsigned Newlines,
-  unsigned PreviousEndOfTokenColumn,
-  unsigned EscapedNewlineColumn) {
-  if (Newlines > 0) {
+void WhitespaceManager::appendEscapedNewlineText(std::string , const 
Change ) {
+  if (Style.AlignEscapedNewlines == FormatStyle::ENAS_DontAlign) {
+for (unsigned i = 0; i < C.NewlinesBefore; ++i)
+  Text.append(UseCRLF ? " \\\r\n" : " \\\n");
+return;
+  }
+
+  if (C.NewlinesBefore > 0) {
+assert(C.EscapedNewlineColumn >= 1);
 unsigned Offset =
-std::min(EscapedNewlineColumn - 2, PreviousEndOfTokenColumn);
-for (unsigned i = 0; i < Newlines; ++i) {
-  Text.append(EscapedNewlineColumn - Offset - 1, ' ');
+std::min(C.EscapedNewlineColumn - 1, C.PreviousEndOfTokenColumn);
+for (unsigned i = 0; i < C.NewlinesBefore; ++i) {
+  Text.append(C.EscapedNewlineColumn - Offset - 1, ' ');
   Text.append(UseCRLF ? "\\\r\n" : "\\\n");
   Offset = 0;
 }


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2309,6 +2309,30 @@
   EXPECT_EQ("template  f();", format("\\\ntemplate  f();"));
   EXPECT_EQ("/* \\  \\  \\\n */", format("\\\n/* \\  \\  \\\n */"));
   EXPECT_EQ("", format(""));
+
+  FormatStyle DontAlign = 

[PATCH] D36019: [clang-format] Fix bug with ENAS_DontAlign and empty lines

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes created this revision.
Herald added a subscriber: klimek.

This fixes a bug in `ENAS_DontAlign` (introduced in 
https://reviews.llvm.org/D32733) where blank lines had an EscapedNewlineColumn 
of 0, causing a subtraction to overflow when converted back to `unsigned` and 
leading to runaway memory allocation.

This restores the original approach of a separate loop as originally proposed 
in https://reviews.llvm.org/D32733?vs=97397=97404, now with a proper 
justification :)


https://reviews.llvm.org/D36019

Files:
  lib/Format/WhitespaceManager.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2309,6 +2309,30 @@
   EXPECT_EQ("template  f();", format("\\\ntemplate  f();"));
   EXPECT_EQ("/* \\  \\  \\\n */", format("\\\n/* \\  \\  \\\n */"));
   EXPECT_EQ("", format(""));
+
+  FormatStyle DontAlign = getLLVMStyle();
+  DontAlign.AlignEscapedNewlines = FormatStyle::ENAS_DontAlign;
+  DontAlign.MaxEmptyLinesToKeep = 3;
+  // FIXME: can't use verifyFormat here because the newline before
+  // "public:" is not inserted the first time it's reformatted
+  EXPECT_EQ("#define A \\\n"
+"  class Foo { \\\n"
+"void bar(); \\\n"
+" \\\n"
+" \\\n"
+" \\\n"
+"  public: \\\n"
+"void baz(); \\\n"
+"  };",
+format("#define A \\\n"
+   "  class Foo { \\\n"
+   "void bar(); \\\n"
+   " \\\n"
+   " \\\n"
+   " \\\n"
+   "  public: \\\n"
+   "void baz(); \\\n"
+   "  };", DontAlign));
 }
 
 TEST_F(FormatTest, CalculateSpaceOnConsecutiveLinesInMacro) {
Index: lib/Format/WhitespaceManager.cpp
===
--- lib/Format/WhitespaceManager.cpp
+++ lib/Format/WhitespaceManager.cpp
@@ -557,9 +557,22 @@
 }
 
 void WhitespaceManager::alignEscapedNewlines() {
-  if (Style.AlignEscapedNewlines == FormatStyle::ENAS_DontAlign)
+  // If we are not aligning escaped newlines, just set EscapedNewlineColumn
+  // to point to the end of each line.
+  if (Style.AlignEscapedNewlines == FormatStyle::ENAS_DontAlign) {
+bool PreviousContinuerHadNLBefore = false;  // used to detect blank lines
+for (Change  : Changes) {
+  if (C.ContinuesPPDirective) {
+if (C.NewlinesBefore > 0)
+  C.EscapedNewlineColumn =
+PreviousContinuerHadNLBefore ? 2 : C.PreviousEndOfTokenColumn + 2;
+PreviousContinuerHadNLBefore = C.NewlinesBefore > 0;
+  }
+}
 return;
+  }
 
+  // Otherwise, compute the max width and then apply it to all lines.
   bool AlignLeft = Style.AlignEscapedNewlines == FormatStyle::ENAS_Left;
   unsigned MaxEndOfLine = AlignLeft ? 0 : Style.ColumnLimit;
   unsigned StartOfMacro = 0;
@@ -644,6 +657,7 @@
   unsigned PreviousEndOfTokenColumn,
   unsigned EscapedNewlineColumn) {
   if (Newlines > 0) {
+assert(EscapedNewlineColumn >= 2);
 unsigned Offset =
 std::min(EscapedNewlineColumn - 2, PreviousEndOfTokenColumn);
 for (unsigned i = 0; i < Newlines; ++i) {


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2309,6 +2309,30 @@
   EXPECT_EQ("template  f();", format("\\\ntemplate  f();"));
   EXPECT_EQ("/* \\  \\  \\\n */", format("\\\n/* \\  \\  \\\n */"));
   EXPECT_EQ("", format(""));
+
+  FormatStyle DontAlign = getLLVMStyle();
+  DontAlign.AlignEscapedNewlines = FormatStyle::ENAS_DontAlign;
+  DontAlign.MaxEmptyLinesToKeep = 3;
+  // FIXME: can't use verifyFormat here because the newline before
+  // "public:" is not inserted the first time it's reformatted
+  EXPECT_EQ("#define A \\\n"
+"  class Foo { \\\n"
+"void bar(); \\\n"
+" \\\n"
+" \\\n"
+" \\\n"
+"  public: \\\n"
+"void baz(); \\\n"
+"  };",
+format("#define A \\\n"
+   "  class Foo { \\\n"
+   "void bar(); \\\n"
+   " \\\n"
+   " \\\n"
+   " \\\n"
+   "  public: \\\n"
+   "void baz(); \\\n"
+   "  };", DontAlign));
 }
 
 TEST_F(FormatTest, CalculateSpaceOnConsecutiveLinesInMacro) {
Index: lib/Format/WhitespaceManager.cpp
===
--- lib/Format/WhitespaceManager.cpp
+++ lib/Format/WhitespaceManager.cpp
@@ -557,9 +557,22 @@
 }
 
 void WhitespaceManager::alignEscapedNewlines() {

[PATCH] D34330: [clang-format] handle `if constexpr`

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added a comment.

Thanks for the review. Please note that there was a prior effort to implement 
this in https://reviews.llvm.org/D26953. However if you are happy with this 
version, feel free to commit (as I don’t have commit access).


https://reviews.llvm.org/D34330



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

[PATCH] D26953: clang-format: handle formatting on constexpr if

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added a comment.

Hm, I probably should've searched first — but I just re-implemented this in 
https://reviews.llvm.org/D34330.  Actually, I think my implementation solves 
the `AllowShortIfStatementsOnASingleLine` issue you were mentioning here 


https://reviews.llvm.org/D26953



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

[PATCH] D34330: [clang-format] handle `if constexpr`

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes created this revision.
Herald added a subscriber: klimek.

Changes to handle `if constexpr` the same way as `if`.


https://reviews.llvm.org/D34330

Files:
  lib/Format/ContinuationIndenter.cpp
  lib/Format/TokenAnnotator.cpp
  lib/Format/UnwrappedLineParser.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -341,6 +341,18 @@
   verifyFormat("if (true)\n  f();\ng();");
   verifyFormat("if (a)\n  if (b)\nif (c)\n  g();\nh();");
   verifyFormat("if (a)\n  if (b) {\nf();\n  }\ng();");
+  verifyFormat("if constexpr (true)\n"
+   "  f();\ng();");
+  verifyFormat("if constexpr (a)\n"
+   "  if constexpr (b)\n"
+   "if constexpr (c)\n"
+   "  g();\n"
+   "h();");
+  verifyFormat("if constexpr (a)\n"
+   "  if constexpr (b) {\n"
+   "f();\n"
+   "  }\n"
+   "g();");
 
   FormatStyle AllowsMergedIf = getLLVMStyle();
   AllowsMergedIf.AlignEscapedNewlines = FormatStyle::ENAS_Left;
@@ -423,9 +435,11 @@
   AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = true;
 
   verifyFormat("if (true) {}", AllowSimpleBracedStatements);
+  verifyFormat("if constexpr (true) {}", AllowSimpleBracedStatements);
   verifyFormat("while (true) {}", AllowSimpleBracedStatements);
   verifyFormat("for (;;) {}", AllowSimpleBracedStatements);
   verifyFormat("if (true) { f(); }", AllowSimpleBracedStatements);
+  verifyFormat("if constexpr (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("while (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("if (true) { //\n"
@@ -496,6 +510,19 @@
"else {\n"
"  i();\n"
"}");
+  verifyFormat("if (true)\n"
+   "  if constexpr (true)\n"
+   "if (true) {\n"
+   "  if constexpr (true)\n"
+   "f();\n"
+   "} else {\n"
+   "  g();\n"
+   "}\n"
+   "  else\n"
+   "h();\n"
+   "else {\n"
+   "  i();\n"
+   "}");
   verifyFormat("void f() {\n"
"  if (a) {\n"
"  } else {\n"
@@ -511,6 +538,12 @@
"  g();\n"
"else\n"
"  h();");
+  verifyFormat("if constexpr (a)\n"
+   "  f();\n"
+   "else if constexpr (b)\n"
+   "  g();\n"
+   "else\n"
+   "  h();");
   verifyFormat("if (a) {\n"
"  f();\n"
"}\n"
@@ -528,6 +561,11 @@
"a) {\n"
"}",
getLLVMStyleWithColumns(62));
+  verifyFormat("if (a) {\n"
+   "} else if constexpr (\n"
+   "a) {\n"
+   "}",
+   getLLVMStyleWithColumns(62));
 }
 
 TEST_F(FormatTest, FormatsForLoop) {
@@ -2371,6 +2409,9 @@
   verifyFormat("if ((aa ||\n"
" bb) && // \n"
"cc) {\n}");
+  verifyFormat("if constexpr ((aa ||\n"
+   "   bb) && // aaa\n"
+   "  cc) {\n}");
   verifyFormat("b = a &&\n"
"// Comment\n"
"b.c && d;");
@@ -6492,6 +6533,10 @@
"  if (true) continue;\n"
"}",
ShortMergedIf);
+  ShortMergedIf.ColumnLimit = 33;
+  verifyFormat("#define A \\\n"
+   "  if constexpr (true) return 42;",
+   ShortMergedIf);
   ShortMergedIf.ColumnLimit = 29;
   verifyFormat("#define A   \\\n"
"  if (aa) return 1; \\\n"
@@ -6503,6 +6548,11 @@
"return 1; \\\n"
"  return 2;",
ShortMergedIf);
+  verifyFormat("#define A\\\n"
+   "  if constexpr (aaa) \\\n"
+   "return 1;\\\n"
+   "  return 2;",
+   ShortMergedIf);
 }
 
 TEST_F(FormatTest, FormatStarDependingOnContext) {
@@ -8712,11 +8762,24 @@
BreakBeforeBraceShortIfs);
   verifyFormat("void f(bool b)\n"
"{\n"
+   "  if constexpr (b)\n"
+   "  {\n"
+   "return;\n"
+   "  }\n"
+   "}\n",
+   BreakBeforeBraceShortIfs);
+  verifyFormat("void f(bool b)\n"
+   "{\n"
"  if (b) return;\n"
"}\n",
BreakBeforeBraceShortIfs);
 

[PATCH] D34324: [clang-format] let PointerAlignment dictate spacing of function ref qualifiers

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes created this revision.
Herald added a subscriber: klimek.

The original changes for ref qualifiers in https://reviews.llvm.org/rL272537 
and https://reviews.llvm.org/rL272548 allowed function const+ref qualifier 
spacing to diverge from the spacing used for variables. It seems more 
consistent for `T const& x;` to match `void foo() const&;`.


https://reviews.llvm.org/D34324

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -4958,7 +4958,8 @@
   verifyFormat("SomeType MemberFunction(const Deleted &) && {}");
   verifyFormat("SomeType MemberFunction(const Deleted &) && final {}");
   verifyFormat("SomeType MemberFunction(const Deleted &) && override {}");
-  verifyFormat("SomeType MemberFunction(const Deleted &) const &;");
+  verifyFormat("void Fn(T const &) const &;");
+  verifyFormat("void Fn(T const volatile &&) const volatile &&;");
   verifyFormat("template \n"
"void F(T) && = delete;",
getGoogleStyle());
@@ -4975,7 +4976,8 @@
   verifyFormat("auto Function(T... t) & -> void {}", AlignLeft);
   verifyFormat("auto Function(T) & -> void {}", AlignLeft);
   verifyFormat("auto Function(T) & -> void;", AlignLeft);
-  verifyFormat("SomeType MemberFunction(const Deleted&) const &;", AlignLeft);
+  verifyFormat("void Fn(T const&) const&;", AlignLeft);
+  verifyFormat("void Fn(T const volatile&&) const volatile&&;", AlignLeft);
 
   FormatStyle Spaces = getLLVMStyle();
   Spaces.SpacesInCStyleCastParentheses = true;
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2155,8 +2155,7 @@
 return false;
   if (Right.is(TT_PointerOrReference))
 return (Left.is(tok::r_paren) && Line.MightBeFunctionDecl) ||
-   (Left.Tok.isLiteral() || (Left.is(tok::kw_const) && Left.Previous &&
- Left.Previous->is(tok::r_paren)) ||
+   (Left.Tok.isLiteral() ||
 (!Left.isOneOf(TT_PointerOrReference, tok::l_paren) &&
  (Style.PointerAlignment != FormatStyle::PAS_Left ||
   (Line.IsMultiVariableDeclStmt &&


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -4958,7 +4958,8 @@
   verifyFormat("SomeType MemberFunction(const Deleted &) && {}");
   verifyFormat("SomeType MemberFunction(const Deleted &) && final {}");
   verifyFormat("SomeType MemberFunction(const Deleted &) && override {}");
-  verifyFormat("SomeType MemberFunction(const Deleted &) const &;");
+  verifyFormat("void Fn(T const &) const &;");
+  verifyFormat("void Fn(T const volatile &&) const volatile &&;");
   verifyFormat("template \n"
"void F(T) && = delete;",
getGoogleStyle());
@@ -4975,7 +4976,8 @@
   verifyFormat("auto Function(T... t) & -> void {}", AlignLeft);
   verifyFormat("auto Function(T) & -> void {}", AlignLeft);
   verifyFormat("auto Function(T) & -> void;", AlignLeft);
-  verifyFormat("SomeType MemberFunction(const Deleted&) const &;", AlignLeft);
+  verifyFormat("void Fn(T const&) const&;", AlignLeft);
+  verifyFormat("void Fn(T const volatile&&) const volatile&&;", AlignLeft);
 
   FormatStyle Spaces = getLLVMStyle();
   Spaces.SpacesInCStyleCastParentheses = true;
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2155,8 +2155,7 @@
 return false;
   if (Right.is(TT_PointerOrReference))
 return (Left.is(tok::r_paren) && Line.MightBeFunctionDecl) ||
-   (Left.Tok.isLiteral() || (Left.is(tok::kw_const) && Left.Previous &&
- Left.Previous->is(tok::r_paren)) ||
+   (Left.Tok.isLiteral() ||
 (!Left.isOneOf(TT_PointerOrReference, tok::l_paren) &&
  (Style.PointerAlignment != FormatStyle::PAS_Left ||
   (Line.IsMultiVariableDeclStmt &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32825: [clang-format] Improve understanding of combined typedef+record declarations

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes marked an inline comment as done.
jtbandes added a comment.

Another ping.


https://reviews.llvm.org/D32825



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

[PATCH] D32825: [clang-format] Improve understanding of combined typedef+record declarations

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes updated this revision to Diff 98593.
jtbandes added a comment.

Update from review


https://reviews.llvm.org/D32825

Files:
  lib/Format/UnwrappedLineFormatter.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -443,6 +443,14 @@
"}",
AllowSimpleBracedStatements);
 
+  verifyFormat("struct A2 {\n"
+   "  int X;\n"
+   "};",
+   AllowSimpleBracedStatements);
+  verifyFormat("typedef struct A2 {\n"
+   "  int X;\n"
+   "} A2_t;",
+   AllowSimpleBracedStatements);
   verifyFormat("template  struct A2 {\n"
"  struct B {};\n"
"};",
Index: lib/Format/UnwrappedLineFormatter.cpp
===
--- lib/Format/UnwrappedLineFormatter.cpp
+++ lib/Format/UnwrappedLineFormatter.cpp
@@ -365,9 +365,14 @@
 } else if (Limit != 0 && !Line.startsWith(tok::kw_namespace) &&
!startsExternCBlock(Line)) {
   // We don't merge short records.
-  if (Line.First->isOneOf(tok::kw_class, tok::kw_union, tok::kw_struct,
-  Keywords.kw_interface))
-return 0;
+  {
+FormatToken *Tok =
+Line.First->is(tok::kw_typedef) ? Line.First->Next : Line.First;
+if (Tok &&
+Tok->isOneOf(tok::kw_class, tok::kw_union, tok::kw_struct,
+ Keywords.kw_interface))
+  return 0;
+  }
 
   // Check that we still have three lines and they fit into the limit.
   if (I + 2 == E || I[2]->Type == LT_Invalid)


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -443,6 +443,14 @@
"}",
AllowSimpleBracedStatements);
 
+  verifyFormat("struct A2 {\n"
+   "  int X;\n"
+   "};",
+   AllowSimpleBracedStatements);
+  verifyFormat("typedef struct A2 {\n"
+   "  int X;\n"
+   "} A2_t;",
+   AllowSimpleBracedStatements);
   verifyFormat("template  struct A2 {\n"
"  struct B {};\n"
"};",
Index: lib/Format/UnwrappedLineFormatter.cpp
===
--- lib/Format/UnwrappedLineFormatter.cpp
+++ lib/Format/UnwrappedLineFormatter.cpp
@@ -365,9 +365,14 @@
 } else if (Limit != 0 && !Line.startsWith(tok::kw_namespace) &&
!startsExternCBlock(Line)) {
   // We don't merge short records.
-  if (Line.First->isOneOf(tok::kw_class, tok::kw_union, tok::kw_struct,
-  Keywords.kw_interface))
-return 0;
+  {
+FormatToken *Tok =
+Line.First->is(tok::kw_typedef) ? Line.First->Next : Line.First;
+if (Tok &&
+Tok->isOneOf(tok::kw_class, tok::kw_union, tok::kw_struct,
+ Keywords.kw_interface))
+  return 0;
+  }
 
   // Check that we still have three lines and they fit into the limit.
   if (I + 2 == E || I[2]->Type == LT_Invalid)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32825: [clang-format] Improve understanding of combined typedef+record declarations

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes marked an inline comment as done.
jtbandes added inline comments.



Comment at: lib/Format/UnwrappedLineFormatter.cpp:368
   // We don't merge short records.
-  if (Line.First->isOneOf(tok::kw_class, tok::kw_union, tok::kw_struct,
-  Keywords.kw_interface))
+  FormatToken *T =
+  Line.First->is(tok::kw_typedef) ? Line.First->Next : Line.First;

djasper wrote:
> Don't use "T", that's too much engrained as template parameter. Use "Tok".
Added nested scope to avoid shadowing `Tok` from above.


https://reviews.llvm.org/D32825



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

[PATCH] D32825: [clang-format] Improve understanding of combined typedef+record declarations

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added a comment.

Ping :)


https://reviews.llvm.org/D32825



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

[PATCH] D32825: [clang-format] Improve understanding of combined typedef+record declarations

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added a comment.

It strikes me that this doesn't handle `using`-style type aliases, but it seems 
hard to do this correctly in general, so still valuable to catch this simple, 
common case. Let me know if you have any better suggestions!


https://reviews.llvm.org/D32825



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

[PATCH] D32825: [clang-format] Improve understanding of combined typedef+record declarations

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes created this revision.
Herald added a subscriber: klimek.

Fixes an issue where `struct A { int X; };` would be broken onto multiple 
lines, but `typedef struct A { int X; } A2;` was collapsed onto a single line.


https://reviews.llvm.org/D32825

Files:
  lib/Format/UnwrappedLineFormatter.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -443,6 +443,14 @@
"}",
AllowSimpleBracedStatements);
 
+  verifyFormat("struct A2 {\n"
+   "  int X;\n"
+   "};",
+   AllowSimpleBracedStatements);
+  verifyFormat("typedef struct A2 {\n"
+   "  int X;\n"
+   "} A2_t;",
+   AllowSimpleBracedStatements);
   verifyFormat("template  struct A2 {\n"
"  struct B {};\n"
"};",
Index: lib/Format/UnwrappedLineFormatter.cpp
===
--- lib/Format/UnwrappedLineFormatter.cpp
+++ lib/Format/UnwrappedLineFormatter.cpp
@@ -365,8 +365,11 @@
 } else if (Limit != 0 && !Line.startsWith(tok::kw_namespace) &&
!startsExternCBlock(Line)) {
   // We don't merge short records.
-  if (Line.First->isOneOf(tok::kw_class, tok::kw_union, tok::kw_struct,
-  Keywords.kw_interface))
+  FormatToken *T =
+  Line.First->is(tok::kw_typedef) ? Line.First->Next : Line.First;
+  if (T &&
+  T->isOneOf(tok::kw_class, tok::kw_union, tok::kw_struct,
+ Keywords.kw_interface))
 return 0;
 
   // Check that we still have three lines and they fit into the limit.


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -443,6 +443,14 @@
"}",
AllowSimpleBracedStatements);
 
+  verifyFormat("struct A2 {\n"
+   "  int X;\n"
+   "};",
+   AllowSimpleBracedStatements);
+  verifyFormat("typedef struct A2 {\n"
+   "  int X;\n"
+   "} A2_t;",
+   AllowSimpleBracedStatements);
   verifyFormat("template  struct A2 {\n"
"  struct B {};\n"
"};",
Index: lib/Format/UnwrappedLineFormatter.cpp
===
--- lib/Format/UnwrappedLineFormatter.cpp
+++ lib/Format/UnwrappedLineFormatter.cpp
@@ -365,8 +365,11 @@
 } else if (Limit != 0 && !Line.startsWith(tok::kw_namespace) &&
!startsExternCBlock(Line)) {
   // We don't merge short records.
-  if (Line.First->isOneOf(tok::kw_class, tok::kw_union, tok::kw_struct,
-  Keywords.kw_interface))
+  FormatToken *T =
+  Line.First->is(tok::kw_typedef) ? Line.First->Next : Line.First;
+  if (T &&
+  T->isOneOf(tok::kw_class, tok::kw_union, tok::kw_struct,
+ Keywords.kw_interface))
 return 0;
 
   // Check that we still have three lines and they fit into the limit.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32733: [clang-format] Convert AlignEscapedNewlinesLeft to an enum, adding DontAlign

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added a comment.

In https://reviews.llvm.org/D32733#743116, @djasper wrote:

> This is an edge case in actual C++. An escaped newline literally gets 
> expanded to nothing. So what this reads is
>
> #define Onetwo \
>  ...


Yeah, I noticed that, but nonetheless it shouldn't break the alignment of \ in 
the subsequent lines...

Thanks for reviewing! I don't have permissions to commit code; could you do it 
for me?


https://reviews.llvm.org/D32733



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

[PATCH] D32733: [clang-format] Convert AlignEscapedNewlinesLeft to an enum, adding DontAlign

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes marked an inline comment as done.
jtbandes added a comment.

This seems to work fine.

Separately I noticed a strange edge case, which I think is an existing bug:

  #define One\
  two   \
three;  
 \
four;

The lack of space in `One\` seems to break the formatting on the next line. But 
as far as I can tell this isn't related to my change.


https://reviews.llvm.org/D32733



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

[PATCH] D32733: [clang-format] Convert AlignEscapedNewlinesLeft to an enum, adding DontAlign

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes updated this revision to Diff 97404.
jtbandes added a comment.

Modified `appendNewlineText` so we can simply `return` in the DontAlign case.


https://reviews.llvm.org/D32733

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/WhitespaceManager.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/FormatTestSelective.cpp

Index: unittests/Format/FormatTestSelective.cpp
===
--- unittests/Format/FormatTestSelective.cpp
+++ unittests/Format/FormatTestSelective.cpp
@@ -325,7 +325,7 @@
 }
 
 TEST_F(FormatTestSelective, AlwaysFormatsEntireMacroDefinitions) {
-  Style.AlignEscapedNewlinesLeft = true;
+  Style.AlignEscapedNewlines = FormatStyle::ENAS_Left;
   EXPECT_EQ("int  i;\n"
 "#define A \\\n"
 "  int i;  \\\n"
@@ -467,7 +467,7 @@
 TEST_F(FormatTestSelective, UnderstandsTabs) {
   Style.IndentWidth = 8;
   Style.UseTab = FormatStyle::UT_Always;
-  Style.AlignEscapedNewlinesLeft = true;
+  Style.AlignEscapedNewlines = FormatStyle::ENAS_Left;
   EXPECT_EQ("void f() {\n"
 "\tf();\n"
 "\tg();\n"
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -342,7 +342,7 @@
   verifyFormat("if (a)\n  if (b) {\nf();\n  }\ng();");
 
   FormatStyle AllowsMergedIf = getLLVMStyle();
-  AllowsMergedIf.AlignEscapedNewlinesLeft = true;
+  AllowsMergedIf.AlignEscapedNewlines = FormatStyle::ENAS_Left;
   AllowsMergedIf.AllowShortIfStatementsOnASingleLine = true;
   verifyFormat("if (a)\n"
"  // comment\n"
@@ -6423,7 +6423,7 @@
   EXPECT_EQ("\"some text other\";", format("\"some text other\";", Style));
 
   FormatStyle AlignLeft = getLLVMStyleWithColumns(12);
-  AlignLeft.AlignEscapedNewlinesLeft = true;
+  AlignLeft.AlignEscapedNewlines = FormatStyle::ENAS_Left;
   EXPECT_EQ("#define A \\\n"
 "  \"some \" \\\n"
 "  \"text \" \\\n"
@@ -6824,7 +6824,7 @@
   FormatStyle Tab = getLLVMStyleWithColumns(42);
   Tab.IndentWidth = 8;
   Tab.UseTab = FormatStyle::UT_Always;
-  Tab.AlignEscapedNewlinesLeft = true;
+  Tab.AlignEscapedNewlines = FormatStyle::ENAS_Left;
 
   EXPECT_EQ("if ( && // q\n"
 "bb)\t\t// w\n"
@@ -7605,14 +7605,21 @@
"int oneTwoThree = 123;\n"
"int oneTwo = 12;",
Alignment));
-  Alignment.AlignEscapedNewlinesLeft = true;
+  Alignment.AlignEscapedNewlines = FormatStyle::ENAS_DontAlign;
+  verifyFormat("#define A \\\n"
+   "  int    = 12; \\\n"
+   "  int b  = 23; \\\n"
+   "  int ccc= 234; \\\n"
+   "  int dd = 2345;",
+   Alignment);
+  Alignment.AlignEscapedNewlines = FormatStyle::ENAS_Left;
   verifyFormat("#define A   \\\n"
"  int    = 12;  \\\n"
"  int b  = 23;  \\\n"
"  int ccc= 234; \\\n"
"  int dd = 2345;",
Alignment);
-  Alignment.AlignEscapedNewlinesLeft = false;
+  Alignment.AlignEscapedNewlines = FormatStyle::ENAS_Right;
   verifyFormat("#define A  "
"\\\n"
"  int    = 12; "
@@ -7879,14 +7886,21 @@
"}",
Alignment));
   Alignment.AlignConsecutiveAssignments = false;
-  Alignment.AlignEscapedNewlinesLeft = true;
+  Alignment.AlignEscapedNewlines = FormatStyle::ENAS_DontAlign;
+  verifyFormat("#define A \\\n"
+   "  int    = 12; \\\n"
+   "  float b = 23; \\\n"
+   "  const int ccc = 234; \\\n"
+   "  unsigned  dd = 2345;",
+   Alignment);
+  Alignment.AlignEscapedNewlines = FormatStyle::ENAS_Left;
   verifyFormat("#define A  \\\n"
"  int    = 12; \\\n"
"  float b = 23;\\\n"
"  const int ccc = 234; \\\n"
"  unsigned  dd = 2345;",
Alignment);
-  Alignment.AlignEscapedNewlinesLeft = false;
+  Alignment.AlignEscapedNewlines = FormatStyle::ENAS_Right;
   Alignment.ColumnLimit = 30;
   verifyFormat("#define A\\\n"
"  int    = 12;   \\\n"
@@ -8671,7 +8685,6 @@
 TEST_F(FormatTest, ParsesConfigurationBools) {
   FormatStyle Style = {};
   Style.Language = FormatStyle::LK_Cpp;
-  CHECK_PARSE_BOOL(AlignEscapedNewlinesLeft);
   CHECK_PARSE_BOOL(AlignOperands);
   CHECK_PARSE_BOOL(AlignTrailingComments);
   CHECK_PARSE_BOOL(AlignConsecutiveAssignments);
@@ -8794,6 +8807,19 @@
   CHECK_PARSE("AlignAfterOpenBracket: true", AlignAfterOpenBracket,
   FormatStyle::BAS_Align);
 
+  

[PATCH] D32733: [clang-format] Convert AlignEscapedNewlinesLeft to an enum, adding DontAlign

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added inline comments.



Comment at: lib/Format/WhitespaceManager.cpp:523
+  if (C.NewlinesBefore > 0 && C.ContinuesPPDirective)
+C.EscapedNewlineColumn = C.PreviousEndOfTokenColumn + 2;
+return;

djasper wrote:
> I think we should not duplicate this loop. Two alternatives:
> 1. Move this into the other loop. As long as you reset StartOfMacro in each 
> iteration, it should do the right thing.
> 2. Make this work if we just return here. In theory, the "\" should not need 
> any special-casing with this style.
> 
> I'd prefer #2.
I first tried returning here, but the backslashes were butting up against the 
content, as in `int x = foo;\`. I can look around to see if that's fixable.


https://reviews.llvm.org/D32733



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

[PATCH] D32733: [clang-format] Convert AlignEscapedNewlinesLeft to an enum, adding DontAlign

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes created this revision.
Herald added a subscriber: klimek.

This converts the clang-format option `AlignEscapedNewlinesLeft` from a boolean 
to an enum, named `AlignEscapedNewlines`, with options `Left` (prev. `true`), 
`Right` (prev. `false`), and a new option `DontAlign`.

When set to `DontAlign`, the backslashes are placed just after the last token 
in each line:

  #define EXAMPLE \
  do { \
  int x = a; \
  int b; \
  int dd; \
  } while (0)


https://reviews.llvm.org/D32733

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/WhitespaceManager.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/FormatTestSelective.cpp

Index: unittests/Format/FormatTestSelective.cpp
===
--- unittests/Format/FormatTestSelective.cpp
+++ unittests/Format/FormatTestSelective.cpp
@@ -325,7 +325,7 @@
 }
 
 TEST_F(FormatTestSelective, AlwaysFormatsEntireMacroDefinitions) {
-  Style.AlignEscapedNewlinesLeft = true;
+  Style.AlignEscapedNewlines = FormatStyle::ENAS_Left;
   EXPECT_EQ("int  i;\n"
 "#define A \\\n"
 "  int i;  \\\n"
@@ -467,7 +467,7 @@
 TEST_F(FormatTestSelective, UnderstandsTabs) {
   Style.IndentWidth = 8;
   Style.UseTab = FormatStyle::UT_Always;
-  Style.AlignEscapedNewlinesLeft = true;
+  Style.AlignEscapedNewlines = FormatStyle::ENAS_Left;
   EXPECT_EQ("void f() {\n"
 "\tf();\n"
 "\tg();\n"
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -342,7 +342,7 @@
   verifyFormat("if (a)\n  if (b) {\nf();\n  }\ng();");
 
   FormatStyle AllowsMergedIf = getLLVMStyle();
-  AllowsMergedIf.AlignEscapedNewlinesLeft = true;
+  AllowsMergedIf.AlignEscapedNewlines = FormatStyle::ENAS_Left;
   AllowsMergedIf.AllowShortIfStatementsOnASingleLine = true;
   verifyFormat("if (a)\n"
"  // comment\n"
@@ -6423,7 +6423,7 @@
   EXPECT_EQ("\"some text other\";", format("\"some text other\";", Style));
 
   FormatStyle AlignLeft = getLLVMStyleWithColumns(12);
-  AlignLeft.AlignEscapedNewlinesLeft = true;
+  AlignLeft.AlignEscapedNewlines = FormatStyle::ENAS_Left;
   EXPECT_EQ("#define A \\\n"
 "  \"some \" \\\n"
 "  \"text \" \\\n"
@@ -6824,7 +6824,7 @@
   FormatStyle Tab = getLLVMStyleWithColumns(42);
   Tab.IndentWidth = 8;
   Tab.UseTab = FormatStyle::UT_Always;
-  Tab.AlignEscapedNewlinesLeft = true;
+  Tab.AlignEscapedNewlines = FormatStyle::ENAS_Left;
 
   EXPECT_EQ("if ( && // q\n"
 "bb)\t\t// w\n"
@@ -7605,14 +7605,21 @@
"int oneTwoThree = 123;\n"
"int oneTwo = 12;",
Alignment));
-  Alignment.AlignEscapedNewlinesLeft = true;
+  Alignment.AlignEscapedNewlines = FormatStyle::ENAS_DontAlign;
+  verifyFormat("#define A \\\n"
+   "  int    = 12; \\\n"
+   "  int b  = 23; \\\n"
+   "  int ccc= 234; \\\n"
+   "  int dd = 2345;",
+   Alignment);
+  Alignment.AlignEscapedNewlines = FormatStyle::ENAS_Left;
   verifyFormat("#define A   \\\n"
"  int    = 12;  \\\n"
"  int b  = 23;  \\\n"
"  int ccc= 234; \\\n"
"  int dd = 2345;",
Alignment);
-  Alignment.AlignEscapedNewlinesLeft = false;
+  Alignment.AlignEscapedNewlines = FormatStyle::ENAS_Right;
   verifyFormat("#define A  "
"\\\n"
"  int    = 12; "
@@ -7879,14 +7886,21 @@
"}",
Alignment));
   Alignment.AlignConsecutiveAssignments = false;
-  Alignment.AlignEscapedNewlinesLeft = true;
+  Alignment.AlignEscapedNewlines = FormatStyle::ENAS_DontAlign;
+  verifyFormat("#define A \\\n"
+   "  int    = 12; \\\n"
+   "  float b = 23; \\\n"
+   "  const int ccc = 234; \\\n"
+   "  unsigned  dd = 2345;",
+   Alignment);
+  Alignment.AlignEscapedNewlines = FormatStyle::ENAS_Left;
   verifyFormat("#define A  \\\n"
"  int    = 12; \\\n"
"  float b = 23;\\\n"
"  const int ccc = 234; \\\n"
"  unsigned  dd = 2345;",
Alignment);
-  Alignment.AlignEscapedNewlinesLeft = false;
+  Alignment.AlignEscapedNewlines = FormatStyle::ENAS_Right;
   Alignment.ColumnLimit = 30;
   verifyFormat("#define A\\\n"
"  int    = 12;   \\\n"
@@ -8671,7 +8685,6 @@
 TEST_F(FormatTest, ParsesConfigurationBools) {
   FormatStyle Style = {};
   

[PATCH] D32475: [clang-format] Don’t propagate AvoidBinPacking into argument subexpressions

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added a comment.

Friendly ping :)  happy to make more changes if needed. Thanks again for your 
time.


https://reviews.llvm.org/D32475



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

[PATCH] D32475: [clang-format] Don’t propagate AvoidBinPacking into argument subexpressions

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added a comment.

@djasper how does this look?

I could try to simplify the examples further, but I feel it's important to have 
calls with many subexpressions to exercise this behavior properly. FWIW, my 
real-world use case is with `<<` appearing as an output stream operator inside 
a macro invocation.


https://reviews.llvm.org/D32475



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

[PATCH] D32475: [clang-format] Don’t propagate AvoidBinPacking into argument subexpressions

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes updated this revision to Diff 96815.
jtbandes marked 6 inline comments as done.
jtbandes added a comment.

Updates from review


https://reviews.llvm.org/D32475

Files:
  lib/Format/ContinuationIndenter.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2591,6 +2591,60 @@
Style);
 }
 
+TEST_F(FormatTest, AllowBinPackingInsideArguments) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;
+  Style.BinPackArguments = false;
+  Style.ColumnLimit = 40;
+  verifyFormat("void test() {\n"
+   "  someFunction(\n"
+   "  this + argument + is + quite\n"
+   "  + long + so + it + gets + wrapped\n"
+   "  + but + remains + bin - packed);\n"
+   "}",
+   Style);
+  verifyFormat("void test() {\n"
+   "  someFunction(arg1,\n"
+   "   this + argument + is\n"
+   "   + quite + long + so\n"
+   "   + it + gets + wrapped\n"
+   "   + but + remains + bin\n"
+   "   - packed,\n"
+   "   arg3);\n"
+   "}",
+   Style);
+  verifyFormat("void test() {\n"
+   "  someFunction(\n"
+   "  arg1,\n"
+   "  this + argument + has\n"
+   "  + anotherFunc(nested,\n"
+   "calls + whose\n"
+   "+ arguments\n"
+   "+ are + also\n"
+   "+ wrapped,\n"
+   "in + addition)\n"
+   "  + to + being + bin - packed,\n"
+   "  arg3);\n"
+   "}",
+   Style);
+
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_None;
+  verifyFormat("void test() {\n"
+   "  someFunction(\n"
+   "  arg1,\n"
+   "  this + argument + has +\n"
+   "  anotherFunc(nested,\n"
+   "  calls + whose +\n"
+   "  arguments +\n"
+   "  are + also +\n"
+   "  wrapped,\n"
+   "  in + addition) +\n"
+   "  to + being + bin - packed,\n"
+   "  arg3);\n"
+   "}",
+   Style);
+}
+
 TEST_F(FormatTest, ConstructorInitializers) {
   verifyFormat("Constructor() : Initializer(FitsOnTheLine) {}");
   verifyFormat("Constructor() : Inttializer(FitsOnTheLine) {}",
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -920,6 +920,10 @@
 NewParenState.NoLineBreak =
 NewParenState.NoLineBreak || State.Stack.back().NoLineBreakInOperand;
 
+// Don't propagate AvoidBinPacking into subexpressions of arg/param lists.
+if (*I > prec::Comma)
+  NewParenState.AvoidBinPacking = false;
+
 // Indent from 'LastSpace' unless these are fake parentheses encapsulating
 // a builder type call after 'return' or, if the alignment after opening
 // brackets is disabled.


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2591,6 +2591,60 @@
Style);
 }
 
+TEST_F(FormatTest, AllowBinPackingInsideArguments) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;
+  Style.BinPackArguments = false;
+  Style.ColumnLimit = 40;
+  verifyFormat("void test() {\n"
+   "  someFunction(\n"
+   "  this + argument + is + quite\n"
+   "  + long + so + it + gets + wrapped\n"
+   "  + but + remains + bin - packed);\n"
+   "}",
+   Style);
+  verifyFormat("void test() {\n"
+   "  someFunction(arg1,\n"
+   "   this + argument + is\n"
+   "   + quite + long + so\n"
+   "   + it + gets + wrapped\n"
+   "   + but + remains + bin\n"
+   "   - packed,\n"
+   "   arg3);\n"
+   "}",
+   Style);
+  verifyFormat("void test() {\n"
+   "  someFunction(\n"
+   "  arg1,\n"
+   "  this + argument + has\n"
+   "  + 

[PATCH] D32475: [clang-format] Don’t propagate AvoidBinPacking into argument subexpressions

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added inline comments.



Comment at: unittests/Format/FormatTest.cpp:2597
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;
+  Style.BinPackArguments = false;

djasper wrote:
> Does this bug only happen when breaking before operators? If not can you add 
> a test case with None?
Yeah it is only when breaking before operators, because the condition which 
causes `mustBreak` to be true includes `Current.CanBreakBefore`.



Comment at: unittests/Format/FormatTest.cpp:2599
+  Style.BinPackArguments = false;
+  Style.BinPackParameters = false;
+  verifyFormat(StringRef(R"(

djasper wrote:
> This is not tested/changed at all, I think.
That's a good point. I had this because my test code is a function call at the 
top level, which is treated as a signature rather than a call. I will wrap it 
in another block and remove the BinPackParameters setting.


https://reviews.llvm.org/D32475



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

[PATCH] D32475: [clang-format] Don’t propagate AvoidBinPacking into argument subexpressions

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added inline comments.



Comment at: lib/Format/ContinuationIndenter.cpp:923
+// Don't propagate AvoidBinPacking into subexpressions of arg/param lists.
+if (Current.FakeLParens.size() > 0 &&
+Current.FakeLParens.back() > prec::Comma) {

jtbandes wrote:
> djasper wrote:
> > I think you cannot get here if .size() is 0 as this is iterating over the 
> > elements.
> Good point, thanks.
Actually, I can probably just use `*I > prec::Comma`.


https://reviews.llvm.org/D32475



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

[PATCH] D32475: [clang-format] Don’t propagate AvoidBinPacking into argument subexpressions

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added a comment.

Thanks for the feedback, I'll work on making those changes.

In https://reviews.llvm.org/D32475#738425, @djasper wrote:

> What happens if the function call where this happens actually does not have 
> multiple parameters but one parameter with many operands, e.g. changing your 
> test case to:
>
>   arg3 + is + quite + long + so + it
>   + f(arguments << of << its << subexpressions << lengthy << as << they
> << may << or__ << may)


I don't think this is relevant to what I am trying to fix. The issue being 
fixed is specifically with multiple arguments: I don't want 
BinPackArguments=false to affect bin-packing of an individual argument. For 
completeness, though, I can include a test case with one argument.




Comment at: lib/Format/ContinuationIndenter.cpp:923
+// Don't propagate AvoidBinPacking into subexpressions of arg/param lists.
+if (Current.FakeLParens.size() > 0 &&
+Current.FakeLParens.back() > prec::Comma) {

djasper wrote:
> I think you cannot get here if .size() is 0 as this is iterating over the 
> elements.
Good point, thanks.



Comment at: unittests/Format/FormatTest.cpp:2596
+  FormatStyle Style = getLLVMStyle();
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;

djasper wrote:
> Is this important?
Not really. I can remove it.


https://reviews.llvm.org/D32475



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

[PATCH] D32475: [clang-format] Don’t propagate AvoidBinPacking into argument subexpressions

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes added a reviewer: bkramer.
jtbandes added a comment.

Not exactly sure who is the right person for this.


https://reviews.llvm.org/D32475



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

[PATCH] D32333: [clang-tidy] Update IdentifierNamingCheck to remove extra leading/trailing underscores

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes marked an inline comment as done.
jtbandes added a comment.

Done, thanks for the review!

What is the procedure for merging patches in? I'm sure I don't have permissions 
to do it myself.


https://reviews.llvm.org/D32333



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

[PATCH] D32333: [clang-tidy] Update IdentifierNamingCheck to remove extra leading/trailing underscores

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes updated this revision to Diff 96762.
jtbandes added a comment.

Fixed nit


https://reviews.llvm.org/D32333

Files:
  clang-tidy/readability/IdentifierNamingCheck.cpp
  test/clang-tidy/readability-identifier-naming.cpp


Index: test/clang-tidy/readability-identifier-naming.cpp
===
--- test/clang-tidy/readability-identifier-naming.cpp
+++ test/clang-tidy/readability-identifier-naming.cpp
@@ -175,6 +175,9 @@
   int member2 = 2;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for private 
member 'member2'
 // CHECK-FIXES: {{^}}  int __member2 = 2;{{$}}
+  int _memberWithExtraUnderscores_ = 42;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for private 
member '_memberWithExtraUnderscores_'
+// CHECK-FIXES: {{^}}  int __memberWithExtraUnderscores = 42;{{$}}
 
 private:
 int private_member = 3;
Index: clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -262,6 +262,11 @@
   else
 Matches = false;
 
+  // Ensure the name doesn't have any extra underscores beyond those specified
+  // in the prefix and suffix.
+  if (Name.startswith("_") || Name.endswith("_"))
+Matches = false;
+
   if (Style.Case && !Matchers[static_cast(*Style.Case)].match(Name))
 Matches = false;
 
@@ -367,10 +372,12 @@
 static std::string
 fixupWithStyle(StringRef Name,
const IdentifierNamingCheck::NamingStyle ) {
-  return Style.Prefix +
- fixupWithCase(Name, Style.Case.getValueOr(
- IdentifierNamingCheck::CaseType::CT_AnyCase)) 
+
- Style.Suffix;
+  const std::string Fixed = fixupWithCase(
+  Name, 
Style.Case.getValueOr(IdentifierNamingCheck::CaseType::CT_AnyCase));
+  StringRef Mid = StringRef(Fixed).trim("_");
+  if (Mid.empty())
+Mid = "_";
+  return (Style.Prefix + Mid + Style.Suffix).str();
 }
 
 static StyleKind findStyleKind(


Index: test/clang-tidy/readability-identifier-naming.cpp
===
--- test/clang-tidy/readability-identifier-naming.cpp
+++ test/clang-tidy/readability-identifier-naming.cpp
@@ -175,6 +175,9 @@
   int member2 = 2;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for private member 'member2'
 // CHECK-FIXES: {{^}}  int __member2 = 2;{{$}}
+  int _memberWithExtraUnderscores_ = 42;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for private member '_memberWithExtraUnderscores_'
+// CHECK-FIXES: {{^}}  int __memberWithExtraUnderscores = 42;{{$}}
 
 private:
 int private_member = 3;
Index: clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -262,6 +262,11 @@
   else
 Matches = false;
 
+  // Ensure the name doesn't have any extra underscores beyond those specified
+  // in the prefix and suffix.
+  if (Name.startswith("_") || Name.endswith("_"))
+Matches = false;
+
   if (Style.Case && !Matchers[static_cast(*Style.Case)].match(Name))
 Matches = false;
 
@@ -367,10 +372,12 @@
 static std::string
 fixupWithStyle(StringRef Name,
const IdentifierNamingCheck::NamingStyle ) {
-  return Style.Prefix +
- fixupWithCase(Name, Style.Case.getValueOr(
- IdentifierNamingCheck::CaseType::CT_AnyCase)) +
- Style.Suffix;
+  const std::string Fixed = fixupWithCase(
+  Name, Style.Case.getValueOr(IdentifierNamingCheck::CaseType::CT_AnyCase));
+  StringRef Mid = StringRef(Fixed).trim("_");
+  if (Mid.empty())
+Mid = "_";
+  return (Style.Prefix + Mid + Style.Suffix).str();
 }
 
 static StyleKind findStyleKind(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32475: [clang-format] Don’t propagate AvoidBinPacking into argument subexpressions

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes created this revision.
Herald added a subscriber: klimek.

This is an attempt to fix the issue described in my recent email: 
http://lists.llvm.org/pipermail/cfe-dev/2017-April/053632.html

After digging in for a while, I learned that:

- the spurious line breaks were occurring inside the condition 
`Current.is(TT_BinaryOperator) && Current.CanBreakBefore && 
State.Stack.back().BreakBeforeParameter`.
- `BreakBeforeParameter` was being set to true because `AvoidBinPacking` was 
true.
- `AvoidBinPacking` was true because it propagated all the way along the stack 
starting from the `(` "scope opener".

Given the above, it seemed to make sense to reset `AvoidBinPacking` to `false` 
when propagating it into a subexpression.

 hello @djasper — you seem to be the most prolific clang-formatter — please 
feel free to tear this apart 


https://reviews.llvm.org/D32475

Files:
  lib/Format/ContinuationIndenter.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2591,6 +2591,30 @@
Style);
 }
 
+TEST_F(FormatTest, AllowBinPackingInsideArguments) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;
+  Style.BinPackArguments = false;
+  Style.BinPackParameters = false;
+  verifyFormat(StringRef(R"(
+someFunction(
+arg1,
+arg2,
+arg3 + is + quite + long + so + it
++ f(and_even,
+the,
+arguments << of << its << subexpressions << lengthy << as << they
+  << may << or__ << may,
+not__,
+be)
++ a + r + e / l + o + v + i + n + g + l + y / b + i + n - p + a + c + k
++ e + d,
+arg4,
+arg5);
+   )").trim(), Style);
+}
+
 TEST_F(FormatTest, ConstructorInitializers) {
   verifyFormat("Constructor() : Initializer(FitsOnTheLine) {}");
   verifyFormat("Constructor() : Inttializer(FitsOnTheLine) {}",
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -919,6 +919,12 @@
 NewParenState.NoLineBreak =
 NewParenState.NoLineBreak || State.Stack.back().NoLineBreakInOperand;
 
+// Don't propagate AvoidBinPacking into subexpressions of arg/param lists.
+if (Current.FakeLParens.size() > 0 &&
+Current.FakeLParens.back() > prec::Comma) {
+  NewParenState.AvoidBinPacking = false;
+}
+
 // Indent from 'LastSpace' unless these are fake parentheses encapsulating
 // a builder type call after 'return' or, if the alignment after opening
 // brackets is disabled.


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2591,6 +2591,30 @@
Style);
 }
 
+TEST_F(FormatTest, AllowBinPackingInsideArguments) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;
+  Style.BinPackArguments = false;
+  Style.BinPackParameters = false;
+  verifyFormat(StringRef(R"(
+someFunction(
+arg1,
+arg2,
+arg3 + is + quite + long + so + it
++ f(and_even,
+the,
+arguments << of << its << subexpressions << lengthy << as << they
+  << may << or__ << may,
+not__,
+be)
++ a + r + e / l + o + v + i + n + g + l + y / b + i + n - p + a + c + k
++ e + d,
+arg4,
+arg5);
+   )").trim(), Style);
+}
+
 TEST_F(FormatTest, ConstructorInitializers) {
   verifyFormat("Constructor() : Initializer(FitsOnTheLine) {}");
   verifyFormat("Constructor() : Inttializer(FitsOnTheLine) {}",
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -919,6 +919,12 @@
 NewParenState.NoLineBreak =
 NewParenState.NoLineBreak || State.Stack.back().NoLineBreakInOperand;
 
+// Don't propagate AvoidBinPacking into subexpressions of arg/param lists.
+if (Current.FakeLParens.size() > 0 &&
+Current.FakeLParens.back() > prec::Comma) {
+  NewParenState.AvoidBinPacking = false;
+}
+
 // Indent from 'LastSpace' unless these are fake parentheses encapsulating
 // a builder type call after 'return' or, if the alignment after opening
 // brackets is disabled.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D32333: [clang-tidy] Update IdentifierNamingCheck to remove extra leading/trailing underscores

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes updated this revision to Diff 96181.
jtbandes added a comment.

Cleanup


https://reviews.llvm.org/D32333

Files:
  clang-tidy/readability/IdentifierNamingCheck.cpp
  test/clang-tidy/readability-identifier-naming.cpp


Index: test/clang-tidy/readability-identifier-naming.cpp
===
--- test/clang-tidy/readability-identifier-naming.cpp
+++ test/clang-tidy/readability-identifier-naming.cpp
@@ -175,6 +175,9 @@
   int member2 = 2;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for private 
member 'member2'
 // CHECK-FIXES: {{^}}  int __member2 = 2;{{$}}
+  int _memberWithExtraUnderscores_ = 42;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for private 
member '_memberWithExtraUnderscores_'
+// CHECK-FIXES: {{^}}  int __memberWithExtraUnderscores = 42;{{$}}
 
 private:
 int private_member = 3;
Index: clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -262,6 +262,11 @@
   else
 Matches = false;
 
+  // Ensure the name doesn't have any extra underscores beyond those specified
+  // in the prefix and suffix.
+  if (Name.startswith("_") || Name.endswith("_"))
+Matches = false;
+
   if (Style.Case && !Matchers[static_cast(*Style.Case)].match(Name))
 Matches = false;
 
@@ -367,10 +372,12 @@
 static std::string
 fixupWithStyle(StringRef Name,
const IdentifierNamingCheck::NamingStyle ) {
-  return Style.Prefix +
- fixupWithCase(Name, Style.Case.getValueOr(
- IdentifierNamingCheck::CaseType::CT_AnyCase)) 
+
- Style.Suffix;
+  const std::string Fixed = fixupWithCase(
+  Name, 
Style.Case.getValueOr(IdentifierNamingCheck::CaseType::CT_AnyCase));
+  StringRef Mid = StringRef(Fixed).trim("_");
+  if (Mid.size() == 0)
+Mid = "_";
+  return (Style.Prefix + Mid + Style.Suffix).str();
 }
 
 static StyleKind findStyleKind(


Index: test/clang-tidy/readability-identifier-naming.cpp
===
--- test/clang-tidy/readability-identifier-naming.cpp
+++ test/clang-tidy/readability-identifier-naming.cpp
@@ -175,6 +175,9 @@
   int member2 = 2;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for private member 'member2'
 // CHECK-FIXES: {{^}}  int __member2 = 2;{{$}}
+  int _memberWithExtraUnderscores_ = 42;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for private member '_memberWithExtraUnderscores_'
+// CHECK-FIXES: {{^}}  int __memberWithExtraUnderscores = 42;{{$}}
 
 private:
 int private_member = 3;
Index: clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -262,6 +262,11 @@
   else
 Matches = false;
 
+  // Ensure the name doesn't have any extra underscores beyond those specified
+  // in the prefix and suffix.
+  if (Name.startswith("_") || Name.endswith("_"))
+Matches = false;
+
   if (Style.Case && !Matchers[static_cast(*Style.Case)].match(Name))
 Matches = false;
 
@@ -367,10 +372,12 @@
 static std::string
 fixupWithStyle(StringRef Name,
const IdentifierNamingCheck::NamingStyle ) {
-  return Style.Prefix +
- fixupWithCase(Name, Style.Case.getValueOr(
- IdentifierNamingCheck::CaseType::CT_AnyCase)) +
- Style.Suffix;
+  const std::string Fixed = fixupWithCase(
+  Name, Style.Case.getValueOr(IdentifierNamingCheck::CaseType::CT_AnyCase));
+  StringRef Mid = StringRef(Fixed).trim("_");
+  if (Mid.size() == 0)
+Mid = "_";
+  return (Style.Prefix + Mid + Style.Suffix).str();
 }
 
 static StyleKind findStyleKind(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32333: [clang-tidy] Update IdentifierNamingCheck to remove extra leading/trailing underscores

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes updated this revision to Diff 96180.
jtbandes edited the summary of this revision.
jtbandes added a comment.

Remove unnecessary checks for empty prefix/suffix


https://reviews.llvm.org/D32333

Files:
  clang-tidy/readability/IdentifierNamingCheck.cpp
  test/clang-tidy/readability-identifier-naming.cpp


Index: test/clang-tidy/readability-identifier-naming.cpp
===
--- test/clang-tidy/readability-identifier-naming.cpp
+++ test/clang-tidy/readability-identifier-naming.cpp
@@ -175,6 +175,9 @@
   int member2 = 2;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for private 
member 'member2'
 // CHECK-FIXES: {{^}}  int __member2 = 2;{{$}}
+  int _memberWithExtraUnderscores_ = 42;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for private 
member '_memberWithExtraUnderscores_'
+// CHECK-FIXES: {{^}}  int __memberWithExtraUnderscores = 42;{{$}}
 
 private:
 int private_member = 3;
Index: clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -262,6 +262,13 @@
   else
 Matches = false;
 
+  // Ensure the name doesn't have any extra underscores beyond those specified
+  // in the prefix and suffix.
+  if (Name.startswith("_"))
+Matches = false;
+  if (Name.endswith("_"))
+Matches = false;
+
   if (Style.Case && !Matchers[static_cast(*Style.Case)].match(Name))
 Matches = false;
 
@@ -367,10 +374,12 @@
 static std::string
 fixupWithStyle(StringRef Name,
const IdentifierNamingCheck::NamingStyle ) {
-  return Style.Prefix +
- fixupWithCase(Name, Style.Case.getValueOr(
- IdentifierNamingCheck::CaseType::CT_AnyCase)) 
+
- Style.Suffix;
+  const std::string Fixed = fixupWithCase(
+  Name, 
Style.Case.getValueOr(IdentifierNamingCheck::CaseType::CT_AnyCase));
+  StringRef Mid = StringRef(Fixed).trim("_");
+  if (Mid.size() == 0)
+Mid = "_";
+  return (Style.Prefix + Mid + Style.Suffix).str();
 }
 
 static StyleKind findStyleKind(


Index: test/clang-tidy/readability-identifier-naming.cpp
===
--- test/clang-tidy/readability-identifier-naming.cpp
+++ test/clang-tidy/readability-identifier-naming.cpp
@@ -175,6 +175,9 @@
   int member2 = 2;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for private member 'member2'
 // CHECK-FIXES: {{^}}  int __member2 = 2;{{$}}
+  int _memberWithExtraUnderscores_ = 42;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for private member '_memberWithExtraUnderscores_'
+// CHECK-FIXES: {{^}}  int __memberWithExtraUnderscores = 42;{{$}}
 
 private:
 int private_member = 3;
Index: clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -262,6 +262,13 @@
   else
 Matches = false;
 
+  // Ensure the name doesn't have any extra underscores beyond those specified
+  // in the prefix and suffix.
+  if (Name.startswith("_"))
+Matches = false;
+  if (Name.endswith("_"))
+Matches = false;
+
   if (Style.Case && !Matchers[static_cast(*Style.Case)].match(Name))
 Matches = false;
 
@@ -367,10 +374,12 @@
 static std::string
 fixupWithStyle(StringRef Name,
const IdentifierNamingCheck::NamingStyle ) {
-  return Style.Prefix +
- fixupWithCase(Name, Style.Case.getValueOr(
- IdentifierNamingCheck::CaseType::CT_AnyCase)) +
- Style.Suffix;
+  const std::string Fixed = fixupWithCase(
+  Name, Style.Case.getValueOr(IdentifierNamingCheck::CaseType::CT_AnyCase));
+  StringRef Mid = StringRef(Fixed).trim("_");
+  if (Mid.size() == 0)
+Mid = "_";
+  return (Style.Prefix + Mid + Style.Suffix).str();
 }
 
 static StyleKind findStyleKind(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32333: [clang-tidy] Update IdentifierNamingCheck to remove extra leading/trailing underscores

[Jacob Bandes-Storch via Phabricator via cfe-commits]

jtbandes created this revision.

The goal of this change is to fix the following suboptimal replacements 
currently suggested by clang-tidy:

  // with MemberPrefix == "_"
  int __foo;  // accepted without complaint

  // with MemberPrefix == "m_"
  int _foo;
  ^~
  m__foo

I fixed this by

- updating `matchesStyle()` to reject names which have a leading underscore 
after a prefix has already been stripped, or a trailing underscore if a suffix 
has already been stripped;
- updating `fixupWithStyle()` to strip leading & trailing underscores before 
adding the user-defined prefix and suffix.

The replacements are now:

  // MemberPrefix == "_"
  int __foo;
  ^~
  _foo

  // MemberPrefix == "m_"
  int _foo;
  ^
  m_foo

Future improvements might elect to add .clang-tidy flags to improve what is 
being stripped. For instance, stripping `m_` could allow `m_foo` to be 
automatically replaced with `_foo`.


https://reviews.llvm.org/D32333

Files:
  clang-tidy/readability/IdentifierNamingCheck.cpp
  test/clang-tidy/readability-identifier-naming.cpp


Index: test/clang-tidy/readability-identifier-naming.cpp
===
--- test/clang-tidy/readability-identifier-naming.cpp
+++ test/clang-tidy/readability-identifier-naming.cpp
@@ -175,6 +175,9 @@
   int member2 = 2;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for private 
member 'member2'
 // CHECK-FIXES: {{^}}  int __member2 = 2;{{$}}
+  int _memberWithExtraUnderscores_ = 42;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for private 
member '_memberWithExtraUnderscores_'
+// CHECK-FIXES: {{^}}  int __memberWithExtraUnderscores = 42;{{$}}
 
 private:
 int private_member = 3;
Index: clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -262,6 +262,13 @@
   else
 Matches = false;
 
+  // Ensure the name doesn't have any extra underscores beyond those specified
+  // in the prefix and suffix.
+  if (!Style.Prefix.empty() && Name.startswith("_"))
+Matches = false;
+  if (!Style.Suffix.empty() && Name.endswith("_"))
+Matches = false;
+
   if (Style.Case && !Matchers[static_cast(*Style.Case)].match(Name))
 Matches = false;
 
@@ -367,10 +374,12 @@
 static std::string
 fixupWithStyle(StringRef Name,
const IdentifierNamingCheck::NamingStyle ) {
-  return Style.Prefix +
- fixupWithCase(Name, Style.Case.getValueOr(
- IdentifierNamingCheck::CaseType::CT_AnyCase)) 
+
- Style.Suffix;
+  const std::string Fixed = fixupWithCase(
+  Name, 
Style.Case.getValueOr(IdentifierNamingCheck::CaseType::CT_AnyCase));
+  StringRef Mid = StringRef(Fixed).trim("_");
+  if (Mid.size() == 0)
+Mid = "_";
+  return (Style.Prefix + Mid + Style.Suffix).str();
 }
 
 static StyleKind findStyleKind(


Index: test/clang-tidy/readability-identifier-naming.cpp
===
--- test/clang-tidy/readability-identifier-naming.cpp
+++ test/clang-tidy/readability-identifier-naming.cpp
@@ -175,6 +175,9 @@
   int member2 = 2;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for private member 'member2'
 // CHECK-FIXES: {{^}}  int __member2 = 2;{{$}}
+  int _memberWithExtraUnderscores_ = 42;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for private member '_memberWithExtraUnderscores_'
+// CHECK-FIXES: {{^}}  int __memberWithExtraUnderscores = 42;{{$}}
 
 private:
 int private_member = 3;
Index: clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -262,6 +262,13 @@
   else
 Matches = false;
 
+  // Ensure the name doesn't have any extra underscores beyond those specified
+  // in the prefix and suffix.
+  if (!Style.Prefix.empty() && Name.startswith("_"))
+Matches = false;
+  if (!Style.Suffix.empty() && Name.endswith("_"))
+Matches = false;
+
   if (Style.Case && !Matchers[static_cast(*Style.Case)].match(Name))
 Matches = false;
 
@@ -367,10 +374,12 @@
 static std::string
 fixupWithStyle(StringRef Name,
const IdentifierNamingCheck::NamingStyle ) {
-  return Style.Prefix +
- fixupWithCase(Name, Style.Case.getValueOr(
- IdentifierNamingCheck::CaseType::CT_AnyCase)) +
- Style.Suffix;
+  const std::string Fixed = fixupWithCase(
+  Name, Style.Case.getValueOr(IdentifierNamingCheck::CaseType::CT_AnyCase));
+  StringRef Mid = StringRef(Fixed).trim("_");
+  if (Mid.size() == 0)
+Mid = "_";
+  return (Style.Prefix + Mid + Style.Suffix).str();
 }
 
 static StyleKind findStyleKind(