subject:"\[PATCH\] D110668\: \[clang\-cl\] Accept `#pragma warning\(disable \: N\)` for some N"

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

2021-10-04 Thread Nico Weber via Phabricator via cfe-commits

thakis added a comment.

Thanks for the feedback. I'll work on a follow-up that implements that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110668

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

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

2021-10-04 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D110668#3038858 , @xbolva00 wrote:

> In D110668#3036361 , @thakis wrote:
>
>> In D110668#3034576 , @xbolva00 
>> wrote:
>>
>>> Please next time give a bit more time to potential reviewers / other folks 
>>> outside your org. The whole lifecycle of this patch (posted - landed) took 
>>> < 24h.
>>
>> Is there anything wrong with the patch?
>>
>> I agree that it's good to let larger changes sit for a bit, but this seems 
>> like a fairly small and inconsequential change to me. Many patches land with 
>> a review time < 24h.
>>
>> In any case, happy to address post-commit review comments too of course.
>
> I think I would prefer to implement such "mapping" in DiagnosticGroups.td 
> instead of current solution. cc @aaron.ballman as well, as he is an 
> exprienced reviewer here.
>
> What I mean,  for example:
>
>   def UnusedParameter : DiagGroup<"unused-parameter", 4100>;

FWIW, I agree that this is a better approach -- I'm uncomfortable having a 
separate list of diagnostic IDs that matter. To me, this information is part of 
the diagnostic itself. Having it listed alongside the diagnostic text also 
means that when we repurpose a diagnostic to have a slightly different meaning 
(which does happen from time to time), there's something more visible in the 
reviewer's face about whether the change also means the diagnostic shouldn't 
map to a CL diagnostic (or should map to a different one). Personally, I would 
prefer to see it surfaced like this:

`def UnusedParameter : DiagGroup<"unused-parameter", [CL<4100>]>;`

so that it accepts a list of alternative IDs for the diagnostic. We currently 
only care about the stable numbering from cl.exe, but there's no reason we 
wouldn't want to use this for other kinds of diagnostic grouping aliases. For 
example, someday we may want to use these same facilities to map between Clang 
diagnostic groups and GCC diagnostic groups where we surfaced different names 
for the group by accident.

In D110668#3040408 , @thakis wrote:

> That's an interesting idea.
>
> Given how seldom this is used, I weakly prefer having actual code for this 
> though: It makes it easy to see all of those mapped flags, and it keeps 
> rarely used things out of tablegen, which is imho a good thing.

If we want to make it easy to see all the mapped flags, we can add a -cc1 
option to dump them out in a more consumable manner, so I think we have 
solutions we can use there. I don't see this as being a rarely used thing 
though -- it's the stable identifier for the diagnostic, so it strongly relates 
to the definition of the diagnostic itself. This comes up in user-facing ways 
like through `#pragma warning`, so it seems worth it to have it in tablegen, at 
least to me.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110668

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

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

2021-10-04 Thread Nico Weber via Phabricator via cfe-commits

thakis added a comment.

> What I mean,  for example:
>
>   def UnusedParameter : DiagGroup<"unused-parameter", 4100>;

That's an interesting idea.

Given how seldom this is used, I weakly prefer having actual code for this 
though: It makes it easy to see all of those mapped flags, and it keeps rarely 
used things out of tablegen, which is imho a good thing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110668

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

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

2021-10-03 Thread Dávid Bolvanský via Phabricator via cfe-commits

xbolva00 added a subscriber: aaron.ballman.
xbolva00 added a comment.

In D110668#3036361 , @thakis wrote:

> In D110668#3034576 , @xbolva00 
> wrote:
>
>> Please next time give a bit more time to potential reviewers / other folks 
>> outside your org. The whole lifecycle of this patch (posted - landed) took < 
>> 24h.
>
> Is there anything wrong with the patch?
>
> I agree that it's good to let larger changes sit for a bit, but this seems 
> like a fairly small and inconsequential change to me. Many patches land with 
> a review time < 24h.
>
> In any case, happy to address post-commit review comments too of course.

I think I would prefer to implement such "mapping" in DiagnosticGroups.td 
instead of current solution. cc @aaron.ballman as well, as he is an exprienced 
reviewer here.

What I mean,  for example:

  def UnusedParameter : DiagGroup<"unused-parameter", 4100>;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110668

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

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

2021-10-01 Thread Nico Weber via Phabricator via cfe-commits

thakis added a comment.

In D110668#3034576 , @xbolva00 wrote:

> Please next time give a bit more time to potential reviewers / other folks 
> outside your org. The whole lifecycle of this patch (posted - landed) took < 
> 24h.

Is there anything wrong with the patch?

I agree that it's good to let larger changes sit for a bit, but this seems like 
a fairly small and inconsequential change to me. Many patches land with a 
review time < 24h.

In any case, happy to address post-commit review comments too of course.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110668

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

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

2021-09-30 Thread Dávid Bolvanský via Phabricator via cfe-commits

xbolva00 added a comment.

Please next time give a bit more time to potential reviewers / other folks 
outside your org. The whole lifecycle of this patch (postee - landed) took < 
24h.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110668

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

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

2021-09-30 Thread Nico Weber via Phabricator via cfe-commits

thakis added a comment.

Thanks for the revert. The problem was that this `continue` was in the wrong 
spot:

  diff --git a/clang/lib/Driver/ToolChains/Clang.cpp 
b/clang/lib/Driver/ToolChains/Clang.cpp
  index 9450e8b154c5..369c12aea523 100644
  --- a/clang/lib/Driver/ToolChains/Clang.cpp
  +++ b/clang/lib/Driver/ToolChains/Clang.cpp
  @@ -5453,8 +5453,8 @@ void Clang::ConstructJob(Compilation , const 
JobAction ,
 if (auto Group = diagGroupFromCLWarningID(WarningNumber)) {
   CmdArgs.push_back(Args.MakeArgString(
   "-Wno-" + DiagnosticIDs::getWarningOptionForGroup(*Group)));
  -continue;
 }
  +  continue;
   }
   A->render(Args, CmdArgs);
 }

The tests didn't catch this because they pass `-###`. In the reland I'm 
including the following additional test that will catch this at test time (it 
checks we don't pass wd flags with an unknown number through unchanged to cc1):

  diff --git a/clang/test/Driver/cl-options.c b/clang/test/Driver/cl-options.c
  index 74dd68753a1f..618be2d230f9 100644
  --- a/clang/test/Driver/cl-options.c
  +++ b/clang/test/Driver/cl-options.c
  @@ -359,6 +359,7 @@
   // Wno: "-Wno-unused-parameter"
   // Wno: "-Wno-dllexport-explicit-instantiation-decl"
   // Wno: "-Wno-deprecated-declarations"
  +// Wno-NOT: "-wd
   
   // Ignored options. Check that we don't get "unused during compilation" 
errors.
   // RUN: %clang_cl /c \


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110668

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

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

2021-09-29 Thread Amy Huang via Phabricator via cfe-commits

akhuang added a comment.

Seems like there are also errors in chrome builds and the llvm compiler-rt 
build, will just revert it for now --


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110668

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

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

2021-09-29 Thread Haowei Wu via Phabricator via cfe-commits

haowei added a comment.

We are seeing a series of weird errors in our windows clang builder after this 
patch landed:

  [1/828] Building CXX object 
compiler-rt\lib\sanitizer_common\CMakeFiles\RTSanitizerCommonNoTermination.x86_64.dir\sanitizer_solaris.cpp.obj
  FAILED: 
compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommonNoTermination.x86_64.dir/sanitizer_solaris.cpp.obj
 
  C:\b\s\w\ir\x\w\staging\llvm_build\.\bin\clang-cl.exe  /nologo -TP 
-DHAVE_RPC_XDR_H=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE 
-D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS 
-D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE 
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS 
-D__func__=__FUNCTION__ 
-IC:\b\s\w\ir\x\w\llvm-project\compiler-rt\lib\sanitizer_common\.. /DWIN32 
/D_WINDOWS   /Zc:inline /Zc:__cplusplus /Zc:strictStrings /Oi /Zc:rvalueCast 
/Brepro /bigobj /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 
-wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 
-wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 
-wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd4324 -w14062 -we4238 
/Gw /MD /Z7 /O2 /Ob1/Zc:threadSafeInit- -Wthread-safety 
-Wthread-safety-reference -Wthread-safety-beta /Z7 -UNDEBUG /showIncludes 
/Focompiler-rt\lib\sanitizer_common\CMakeFiles\RTSanitizerCommonNoTermination.x86_64.dir\sanitizer_solaris.cpp.obj
 
/Fdcompiler-rt\lib\sanitizer_common\CMakeFiles\RTSanitizerCommonNoTermination.x86_64.dir\
 -c 
C:\b\s\w\ir\x\w\llvm-project\compiler-rt\lib\sanitizer_common\sanitizer_solaris.cpp
  error: unknown argument: '-wd4141'
  error: unknown argument: '-wd4146'
  error: unknown argument: '-wd4244'
  error: unknown argument: '-wd4267'
  error: unknown argument: '-wd4291'
  error: unknown argument: '-wd4351'
  error: unknown argument: '-wd4456'
  error: unknown argument: '-wd4457'
  error: unknown argument: '-wd4458'
  error: unknown argument: '-wd4459'
  error: unknown argument: '-wd4503'
  error: unknown argument: '-wd4624'
  error: unknown argument: '-wd4722'
  error: unknown argument: '-wd4127'
  error: unknown argument: '-wd4512'
  error: unknown argument: '-wd4505'
  error: unknown argument: '-wd4610'
  error: unknown argument: '-wd4510'
  error: unknown argument: '-wd4702'
  fatal error: too many errors emitted, stopping now [-ferror-limit=]

Failed build task is 
https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-windows-x64/b8834735134024529217/overview
 . The error disappeared when I revert this change on our builder. Do you have 
an idea why this change caused the build issue?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110668

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

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

2021-09-29 Thread Nico Weber via Phabricator via cfe-commits

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
thakis marked an inline comment as done.
Closed by commit rGb2de52bec17b: [clang-cl] Accept `#pragma warning(disable : 
N)` for some N (authored by thakis).

Changed prior to commit:
  https://reviews.llvm.org/D110668?vs=375891=375942#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110668

Files:
  clang/include/clang/Basic/CLWarnings.h
  clang/include/clang/Basic/Diagnostic.h
  clang/include/clang/Basic/DiagnosticCategories.h
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticIDs.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Basic/CLWarnings.cpp
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Diagnostic.cpp
  clang/lib/Basic/DiagnosticIDs.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Lex/Pragma.cpp
  clang/test/Driver/cl-options.c
  clang/test/Sema/pragma-warning.cpp
  clang/tools/diagtool/DiagnosticNames.cpp
  clang/utils/TableGen/ClangDiagnosticsEmitter.cpp
  llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn

Index: llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
+++ llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
@@ -55,6 +55,7 @@
   sources = [
 "Attributes.cpp",
 "Builtins.cpp",
+"CLWarnings.cpp",
 "CharInfo.cpp",
 "CodeGenOptions.cpp",
 "Cuda.cpp",
Index: clang/utils/TableGen/ClangDiagnosticsEmitter.cpp
===
--- clang/utils/TableGen/ClangDiagnosticsEmitter.cpp
+++ clang/utils/TableGen/ClangDiagnosticsEmitter.cpp
@@ -129,6 +129,7 @@
   };
 
   struct GroupInfo {
+llvm::StringRef GroupName;
 std::vector DiagsInGroup;
 std::vector SubGroups;
 unsigned IDNo;
@@ -174,6 +175,7 @@
 Record *Group = DiagGroups[i];
 GroupInfo  =
 DiagsInGroup[std::string(Group->getValueAsString("GroupName"))];
+GI.GroupName = Group->getName();
 GI.Defs.push_back(Group);
 
 std::vector SubGroups = Group->getValueAsListOfDefs("SubGroups");
@@ -1279,8 +1281,8 @@
 OS << ", \"";
 OS.write_escaped(DiagTextBuilder.buildForDefinition()) << '"';
 
-// Warning associated with the diagnostic. This is stored as an index into
-// the alphabetically sorted warning table.
+// Warning group associated with the diagnostic. This is stored as an index
+// into the alphabetically sorted warning group table.
 if (DefInit *DI = dyn_cast(R.getValueInit("Group"))) {
   std::map::iterator I = DiagsInGroup.find(
   std::string(DI->getDef()->getValueAsString("GroupName")));
@@ -1487,18 +1489,20 @@
   for (auto const : DiagsInGroup)
 MaxLen = std::max(MaxLen, (unsigned)I.first.size());
 
-  OS << "\n#ifdef GET_DIAG_TABLE\n";
+  OS << "\n#ifdef DIAG_ENTRY\n";
   unsigned SubGroupIndex = 1, DiagArrayIndex = 1;
   for (auto const : DiagsInGroup) {
 // Group option string.
-OS << "  { /* ";
+OS << "DIAG_ENTRY(";
+OS << I.second.GroupName << " /* ";
+
 if (I.first.find_first_not_of("abcdefghijklmnopqrstuvwxyz"
"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
"0123456789!@#$%^*-+=:?") !=
 std::string::npos)
   PrintFatalError("Invalid character in diagnostic group '" + I.first +
   "'");
-OS << I.first << " */ " << std::string(MaxLen - I.first.size(), ' ');
+OS << I.first << " */, ";
 // Store a pascal-style length byte at the beginning of the string.
 std::string Name = char(I.first.size()) + I.first;
 OS << GroupNames.GetOrAddStringOffset(Name, false) << ", ";
@@ -1517,7 +1521,7 @@
 DiagArrayIndex += DiagsInPedantic.size();
   DiagArrayIndex += V.size() + 1;
 } else {
-  OS << "/* Empty */ 0, ";
+  OS << "0, ";
 }
 
 // Subgroups.
@@ -1530,12 +1534,12 @@
 SubGroupIndex += GroupsInPedantic.size();
   SubGroupIndex += SubGroups.size() + 1;
 } else {
-  OS << "/* Empty */ 0";
+  OS << "0";
 }
 
-OS << " },\n";
+OS << " )\n";
   }
-  OS << "#endif // GET_DIAG_TABLE\n\n";
+  OS << "#endif // DIAG_ENTRY\n\n";
 }
 
 /// Emit the table of diagnostic categories.
Index: clang/tools/diagtool/DiagnosticNames.cpp
===
--- clang/tools/diagtool/DiagnosticNames.cpp
+++ clang/tools/diagtool/DiagnosticNames.cpp
@@ -66,9 +66,10 @@
 
 // Second the table of options, sorted by name for fast binary lookup.
 static const GroupRecord OptionTable[] = {
-#define GET_DIAG_TABLE
+#define DIAG_ENTRY(GroupName, FlagNameOffset, Members, SubGroups)  \
+  {FlagNameOffset, Members, SubGroups},
 #include

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

2021-09-29 Thread Hans Wennborg via Phabricator via cfe-commits

hans accepted this revision.
hans added a comment.
This revision is now accepted and ready to land.

Nice! lgtm




Comment at: clang/include/clang/Basic/CLWarnings.h:18
+enum class Group;
+};
+

clang-tidy's comment about the extra semicolon seems valid


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

https://reviews.llvm.org/D110668

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

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

2021-09-29 Thread Nico Weber via Phabricator via cfe-commits

thakis updated this revision to Diff 375891.
thakis added a comment.
Herald added subscribers: usaxena95, arphaman.

tblgen group IDs

Done, please take a look. We now have a `diag::Group` enum.


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

https://reviews.llvm.org/D110668

Files:
  clang/include/clang/Basic/CLWarnings.h
  clang/include/clang/Basic/Diagnostic.h
  clang/include/clang/Basic/DiagnosticCategories.h
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticIDs.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Basic/CLWarnings.cpp
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Basic/Diagnostic.cpp
  clang/lib/Basic/DiagnosticIDs.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Lex/Pragma.cpp
  clang/test/Driver/cl-options.c
  clang/test/Sema/pragma-warning.cpp
  clang/tools/diagtool/DiagnosticNames.cpp
  clang/utils/TableGen/ClangDiagnosticsEmitter.cpp
  llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn

Index: llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
+++ llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
@@ -55,6 +55,7 @@
   sources = [
 "Attributes.cpp",
 "Builtins.cpp",
+"CLWarnings.cpp",
 "CharInfo.cpp",
 "CodeGenOptions.cpp",
 "Cuda.cpp",
Index: clang/utils/TableGen/ClangDiagnosticsEmitter.cpp
===
--- clang/utils/TableGen/ClangDiagnosticsEmitter.cpp
+++ clang/utils/TableGen/ClangDiagnosticsEmitter.cpp
@@ -129,6 +129,7 @@
   };
 
   struct GroupInfo {
+llvm::StringRef GroupName;
 std::vector DiagsInGroup;
 std::vector SubGroups;
 unsigned IDNo;
@@ -174,6 +175,7 @@
 Record *Group = DiagGroups[i];
 GroupInfo  =
 DiagsInGroup[std::string(Group->getValueAsString("GroupName"))];
+GI.GroupName = Group->getName();
 GI.Defs.push_back(Group);
 
 std::vector SubGroups = Group->getValueAsListOfDefs("SubGroups");
@@ -1279,8 +1281,8 @@
 OS << ", \"";
 OS.write_escaped(DiagTextBuilder.buildForDefinition()) << '"';
 
-// Warning associated with the diagnostic. This is stored as an index into
-// the alphabetically sorted warning table.
+// Warning group associated with the diagnostic. This is stored as an index
+// into the alphabetically sorted warning group table.
 if (DefInit *DI = dyn_cast(R.getValueInit("Group"))) {
   std::map::iterator I = DiagsInGroup.find(
   std::string(DI->getDef()->getValueAsString("GroupName")));
@@ -1487,18 +1489,20 @@
   for (auto const : DiagsInGroup)
 MaxLen = std::max(MaxLen, (unsigned)I.first.size());
 
-  OS << "\n#ifdef GET_DIAG_TABLE\n";
+  OS << "\n#ifdef DIAG_ENTRY\n";
   unsigned SubGroupIndex = 1, DiagArrayIndex = 1;
   for (auto const : DiagsInGroup) {
 // Group option string.
-OS << "  { /* ";
+OS << "DIAG_ENTRY(";
+OS << I.second.GroupName << " /* ";
+
 if (I.first.find_first_not_of("abcdefghijklmnopqrstuvwxyz"
"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
"0123456789!@#$%^*-+=:?") !=
 std::string::npos)
   PrintFatalError("Invalid character in diagnostic group '" + I.first +
   "'");
-OS << I.first << " */ " << std::string(MaxLen - I.first.size(), ' ');
+OS << I.first << " */, ";
 // Store a pascal-style length byte at the beginning of the string.
 std::string Name = char(I.first.size()) + I.first;
 OS << GroupNames.GetOrAddStringOffset(Name, false) << ", ";
@@ -1517,7 +1521,7 @@
 DiagArrayIndex += DiagsInPedantic.size();
   DiagArrayIndex += V.size() + 1;
 } else {
-  OS << "/* Empty */ 0, ";
+  OS << "0, ";
 }
 
 // Subgroups.
@@ -1530,12 +1534,12 @@
 SubGroupIndex += GroupsInPedantic.size();
   SubGroupIndex += SubGroups.size() + 1;
 } else {
-  OS << "/* Empty */ 0";
+  OS << "0";
 }
 
-OS << " },\n";
+OS << " )\n";
   }
-  OS << "#endif // GET_DIAG_TABLE\n\n";
+  OS << "#endif // DIAG_ENTRY\n\n";
 }
 
 /// Emit the table of diagnostic categories.
Index: clang/tools/diagtool/DiagnosticNames.cpp
===
--- clang/tools/diagtool/DiagnosticNames.cpp
+++ clang/tools/diagtool/DiagnosticNames.cpp
@@ -66,9 +66,9 @@
 
 // Second the table of options, sorted by name for fast binary lookup.
 static const GroupRecord OptionTable[] = {
-#define GET_DIAG_TABLE
+#define DIAG_ENTRY(GroupName, FlagNameOffset, Members, SubGroups) { FlagNameOffset, Members, SubGroups },
 #include "clang/Basic/DiagnosticGroups.inc"
-#undef GET_DIAG_TABLE
+#undef DIAG_ENTRY
 };
 
 llvm::StringRef GroupRecord::getName() const {
Index: clang/test/Sema/pragma-warning.cpp

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

2021-09-29 Thread Nico Weber via Phabricator via cfe-commits

thakis added inline comments.



Comment at: clang/lib/Basic/CLWarnings.cpp:17
+  switch (CLWarningID) {
+  case 4005: return "macro-redefined";
+  case 4018: return "sign-compare";

hans wrote:
> Would it be possible to reference the DiagGroup symbolically here somehow 
> instead of using a string? That way, if the DiagGroup gets renamed, we don't 
> risk forgetting to update this string.
As far as I can tell, diagnostic groups don't exist as enums. Options:

- Make this function return a diag:: that the number maps to, and then use 
`DiagnosticIDs::getWarningOptionForDiag()` to get that diag's group name and 
disable that. That should work, but it's a bit confusing since we'd return a 
single diag here (either of warn_deprecated, warn_property_method_deprecated, 
warn_atl_uuid_deprecated and several others would have the same effect of 
representing the group DeprecatedDeclarations) but then disable the whole group.

- Update clang-tblgen to emit something that can be used to create a DiagGroup 
enum and use that.

The latter sounds better to me, so I'll look into that.


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

https://reviews.llvm.org/D110668

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

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

2021-09-29 Thread Hans Wennborg via Phabricator via cfe-commits

hans added a comment.

Nice!




Comment at: clang/lib/Basic/CLWarnings.cpp:17
+  switch (CLWarningID) {
+  case 4005: return "macro-redefined";
+  case 4018: return "sign-compare";

Would it be possible to reference the DiagGroup symbolically here somehow 
instead of using a string? That way, if the DiagGroup gets renamed, we don't 
risk forgetting to update this string.


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

https://reviews.llvm.org/D110668

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

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

2021-09-28 Thread Nico Weber via Phabricator via cfe-commits

thakis updated this revision to Diff 375750.
thakis added a comment.

tweak


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

https://reviews.llvm.org/D110668

Files:
  clang/include/clang/Basic/CLWarnings.h
  clang/include/clang/Driver/Options.td
  clang/lib/Basic/CLWarnings.cpp
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Lex/Pragma.cpp
  clang/test/Driver/cl-options.c
  clang/test/Sema/pragma-warning.cpp
  llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn

Index: llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
+++ llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
@@ -55,6 +55,7 @@
   sources = [
 "Attributes.cpp",
 "Builtins.cpp",
+"CLWarnings.cpp",
 "CharInfo.cpp",
 "CodeGenOptions.cpp",
 "Cuda.cpp",
Index: clang/test/Sema/pragma-warning.cpp
===
--- /dev/null
+++ clang/test/Sema/pragma-warning.cpp
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -fms-extensions -fsyntax-only -verify %s
+
+[[deprecated]] void f() {} // expected-note 2 {{marked deprecated here}}
+
+#define From__pragma() \
+  __pragma(warning(push)) \
+  __pragma(warning(disable:4996)) \
+  f(); \
+  __pragma(warning(pop))
+
+void g() {
+  f(); // expected-warning {{deprecated}}
+
+#pragma warning(push)
+#pragma warning(disable: 4996)
+  f(); // no diag
+
+#pragma warning(disable: 4996)
+#pragma warning(pop)
+
+  f(); // expected-warning {{deprecated}}
+
+  From__pragma(); // no diag
+}
Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -353,7 +353,7 @@
 // CHECK-C11: -std=c11
 
 // For some warning ids, we can map from MSVC warning to Clang warning.
-// RUN: %clang_cl -wd4005 -wd4100 -wd4910 -wd4996 -### -- %s 2>&1 | FileCheck -check-prefix=Wno %s
+// RUN: %clang_cl -wd4005 -wd4100 -wd4910 -wd4996 -wd12345678 -### -- %s 2>&1 | FileCheck -check-prefix=Wno %s
 // Wno: "-cc1"
 // Wno: "-Wno-macro-redefined"
 // Wno: "-Wno-unused-parameter"
Index: clang/lib/Lex/Pragma.cpp
===
--- clang/lib/Lex/Pragma.cpp
+++ clang/lib/Lex/Pragma.cpp
@@ -12,6 +12,7 @@
 //===--===//
 
 #include "clang/Lex/Pragma.h"
+#include "clang/Basic/CLWarnings.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticLex.h"
 #include "clang/Basic/FileManager.h"
@@ -1413,12 +1414,15 @@
   return;
 }
   }
+  PP.getDiagnostics().pushMappings(DiagLoc);
   if (Callbacks)
 Callbacks->PragmaWarningPush(DiagLoc, Level);
 } else if (II && II->isStr("pop")) {
   // #pragma warning( pop )
   PP.Lex(Tok);
-  if (Callbacks)
+  if (!PP.getDiagnostics().popMappings(DiagLoc))
+PP.Diag(Tok, diag::warn_pragma_diagnostic_cannot_pop);
+  else if (Callbacks)
 Callbacks->PragmaWarningPop(DiagLoc);
 } else {
   // #pragma warning( warning-specifier : warning-number-list
@@ -1482,6 +1486,21 @@
   }
   Ids.push_back(int(Value));
 }
+
+// Only act on disable for now.
+diag::Severity SV = diag::Severity();
+if (Specifier == PPCallbacks::PWS_Disable)
+  SV = diag::Severity::Ignored;
+if (SV != diag::Severity())
+  for (int Id : Ids)
+if (const char *Group = diagGroupFromCLWarningID(Id)) {
+  bool unknownDiag = PP.getDiagnostics().setSeverityForGroup(
+  diag::Flavor::WarningOrError, Group, SV, DiagLoc);
+  assert(!unknownDiag &&
+ "wd table should only contain known diags");
+  (void)unknownDiag;
+}
+
 if (Callbacks)
   Callbacks->PragmaWarning(DiagLoc, Specifier, Ids);
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -22,6 +22,7 @@
 #include "Hexagon.h"
 #include "MSP430.h"
 #include "PS4CPU.h"
+#include "clang/Basic/CLWarnings.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/LangOptions.h"
@@ -5443,7 +5444,24 @@
 
   Args.AddAllArgs(CmdArgs, options::OPT_R_Group);
 
-  Args.AddAllArgs(CmdArgs, options::OPT_W_Group);
+  for (const Arg *A :
+   Args.filtered(options::OPT_W_Group, options::OPT__SLASH_wd)) {
+A->claim();
+if (A->getOption().getID() == options::OPT__SLASH_wd) {
+  unsigned WarningNumber;
+  if (StringRef(A->getValue()).getAsInteger(10, WarningNumber)) {
+D.Diag(diag::err_drv_invalid_int_value)
+<< A->getAsString(Args) <<

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

2021-09-28 Thread Nico Weber via Phabricator via cfe-commits

thakis updated this revision to Diff 375749.
thakis added a comment.

some clang-format


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

https://reviews.llvm.org/D110668

Files:
  clang/include/clang/Basic/CLWarnings.h
  clang/include/clang/Driver/Options.td
  clang/lib/Basic/CLWarnings.cpp
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Lex/Pragma.cpp
  clang/test/Driver/cl-options.c
  clang/test/Sema/pragma-warning.cpp
  llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn

Index: llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
+++ llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
@@ -55,6 +55,7 @@
   sources = [
 "Attributes.cpp",
 "Builtins.cpp",
+"CLWarnings.cpp",
 "CharInfo.cpp",
 "CodeGenOptions.cpp",
 "Cuda.cpp",
Index: clang/test/Sema/pragma-warning.cpp
===
--- /dev/null
+++ clang/test/Sema/pragma-warning.cpp
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -fms-extensions -fsyntax-only -verify %s
+
+[[deprecated]] void f() {} // expected-note 2 {{marked deprecated here}}
+
+#define From__pragma() \
+  __pragma(warning(push)) \
+  __pragma(warning(disable:4996)) \
+  f(); \
+  __pragma(warning(pop))
+
+void g() {
+  f(); // expected-warning {{deprecated}}
+
+#pragma warning(push)
+#pragma warning(disable: 4996)
+  f(); // no diag
+
+#pragma warning(disable: 4996)
+#pragma warning(pop)
+
+  f(); // expected-warning {{deprecated}}
+
+  From__pragma(); // no diag
+}
Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -353,7 +353,7 @@
 // CHECK-C11: -std=c11
 
 // For some warning ids, we can map from MSVC warning to Clang warning.
-// RUN: %clang_cl -wd4005 -wd4100 -wd4910 -wd4996 -### -- %s 2>&1 | FileCheck -check-prefix=Wno %s
+// RUN: %clang_cl -wd4005 -wd4100 -wd4910 -wd4996 -wd12345678 -### -- %s 2>&1 | FileCheck -check-prefix=Wno %s
 // Wno: "-cc1"
 // Wno: "-Wno-macro-redefined"
 // Wno: "-Wno-unused-parameter"
Index: clang/lib/Lex/Pragma.cpp
===
--- clang/lib/Lex/Pragma.cpp
+++ clang/lib/Lex/Pragma.cpp
@@ -12,6 +12,7 @@
 //===--===//
 
 #include "clang/Lex/Pragma.h"
+#include "clang/Basic/CLWarnings.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticLex.h"
 #include "clang/Basic/FileManager.h"
@@ -1413,12 +1414,15 @@
   return;
 }
   }
+  PP.getDiagnostics().pushMappings(DiagLoc);
   if (Callbacks)
 Callbacks->PragmaWarningPush(DiagLoc, Level);
 } else if (II && II->isStr("pop")) {
   // #pragma warning( pop )
   PP.Lex(Tok);
-  if (Callbacks)
+  if (!PP.getDiagnostics().popMappings(DiagLoc))
+PP.Diag(Tok, diag::warn_pragma_diagnostic_cannot_pop);
+  else if (Callbacks)
 Callbacks->PragmaWarningPop(DiagLoc);
 } else {
   // #pragma warning( warning-specifier : warning-number-list
@@ -1470,6 +1474,11 @@
   return;
 }
 
+// Only act on disable for now.
+diag::Severity SV;
+if (Specifier == PPCallbacks::PWS_Disable)
+  SV = diag::Severity::Ignored;
+
 // Collect the warning ids.
 SmallVector Ids;
 PP.Lex(Tok);
@@ -1482,6 +1491,15 @@
   }
   Ids.push_back(int(Value));
 }
+if (SV != diag::Severity())
+  for (int Id : Ids)
+if (const char *Group = diagGroupFromCLWarningID(Id)) {
+  bool unknownDiag = PP.getDiagnostics().setSeverityForGroup(
+  diag::Flavor::WarningOrError, Group, SV, DiagLoc);
+  assert(!unknownDiag &&
+ "wd table should only contain known diags");
+  (void)unknownDiag;
+}
 if (Callbacks)
   Callbacks->PragmaWarning(DiagLoc, Specifier, Ids);
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -22,6 +22,7 @@
 #include "Hexagon.h"
 #include "MSP430.h"
 #include "PS4CPU.h"
+#include "clang/Basic/CLWarnings.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/LangOptions.h"
@@ -5443,7 +5444,24 @@
 
   Args.AddAllArgs(CmdArgs, options::OPT_R_Group);
 
-  Args.AddAllArgs(CmdArgs, options::OPT_W_Group);
+  for (const Arg *A :
+   Args.filtered(options::OPT_W_Group, options::OPT__SLASH_wd)) {
+A->claim();
+if (A->getOption().getID() == options::OPT__SLASH_wd) {
+  unsigned WarningNumber;
+  if

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

2021-09-28 Thread Nico Weber via Phabricator via cfe-commits

thakis created this revision.
thakis added a reviewer: hans.
thakis added a project: clang.
Herald added subscribers: dexonsmith, dang, mgorny.
thakis requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

clang-cl maps /wd to -Wno-flags for a few warnings that map
cleanly from cl.exe concepts to clang concepts.

This patch adds support for the same numbers to
`#pragma warning(disable : )`. It also lets
`#pragma warning(push)` and `#pragma warning(pop)` have an effect,
since these are used together with `warning(disable)`.

The optional numeric argument to `warning(push)` is ignored,
as are the other non-`disable` `pragma warning()` arguments.
(Supporting `error` would be easy, but we also don't support
`/we`, and those should probably be added together.)

The motivating example is that a bunch of code (including in LLVM)
uses this idiom to locally disable warnings about calls to deprecated
functions in Windows-only code, and 4996 maps nicely to
-Wno-deprecated-declarations:

  #pragma warning(push)
  #pragma warning(disable: 4996)
f();
  #pragma warning(pop)

Implementation-wise:

- Move `/wd` flag handling from Options.td to actual Driver-level code
- Extract the function mapping cl.exe IDs to warning groups to the new file 
clang/lib/Basic/CLWarnings.cpp
- Call that new function from the PragmaWarning handler


https://reviews.llvm.org/D110668

Files:
  clang/include/clang/Basic/CLWarnings.h
  clang/include/clang/Driver/Options.td
  clang/lib/Basic/CLWarnings.cpp
  clang/lib/Basic/CMakeLists.txt
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Lex/Pragma.cpp
  clang/test/Driver/cl-options.c
  clang/test/Sema/pragma-warning.cpp
  llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn

Index: llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
+++ llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
@@ -55,6 +55,7 @@
   sources = [
 "Attributes.cpp",
 "Builtins.cpp",
+"CLWarnings.cpp",
 "CharInfo.cpp",
 "CodeGenOptions.cpp",
 "Cuda.cpp",
Index: clang/test/Sema/pragma-warning.cpp
===
--- /dev/null
+++ clang/test/Sema/pragma-warning.cpp
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -fms-extensions -fsyntax-only -verify %s
+
+[[deprecated]] void f() {} // expected-note 2 {{marked deprecated here}}
+
+#define From__pragma() \
+  __pragma(warning(push)) \
+  __pragma(warning(disable:4996)) \
+  f(); \
+  __pragma(warning(pop))
+
+void g() {
+  f(); // expected-warning {{deprecated}}
+
+#pragma warning(push)
+#pragma warning(disable: 4996)
+  f(); // no diag
+
+#pragma warning(disable: 4996)
+#pragma warning(pop)
+
+  f(); // expected-warning {{deprecated}}
+
+  From__pragma(); // no diag
+}
Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -353,7 +353,7 @@
 // CHECK-C11: -std=c11
 
 // For some warning ids, we can map from MSVC warning to Clang warning.
-// RUN: %clang_cl -wd4005 -wd4100 -wd4910 -wd4996 -### -- %s 2>&1 | FileCheck -check-prefix=Wno %s
+// RUN: %clang_cl -wd4005 -wd4100 -wd4910 -wd4996 -wd12345678 -### -- %s 2>&1 | FileCheck -check-prefix=Wno %s
 // Wno: "-cc1"
 // Wno: "-Wno-macro-redefined"
 // Wno: "-Wno-unused-parameter"
Index: clang/lib/Lex/Pragma.cpp
===
--- clang/lib/Lex/Pragma.cpp
+++ clang/lib/Lex/Pragma.cpp
@@ -12,6 +12,7 @@
 //===--===//
 
 #include "clang/Lex/Pragma.h"
+#include "clang/Basic/CLWarnings.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticLex.h"
 #include "clang/Basic/FileManager.h"
@@ -1413,12 +1414,15 @@
   return;
 }
   }
+  PP.getDiagnostics().pushMappings(DiagLoc);
   if (Callbacks)
 Callbacks->PragmaWarningPush(DiagLoc, Level);
 } else if (II && II->isStr("pop")) {
   // #pragma warning( pop )
   PP.Lex(Tok);
-  if (Callbacks)
+  if (!PP.getDiagnostics().popMappings(DiagLoc))
+PP.Diag(Tok, diag::warn_pragma_diagnostic_cannot_pop);
+  else if (Callbacks)
 Callbacks->PragmaWarningPop(DiagLoc);
 } else {
   // #pragma warning( warning-specifier : warning-number-list
@@ -1470,6 +1474,11 @@
   return;
 }
 
+// Only act on disable for now.
+diag::Severity SV;
+if (Specifier == PPCallbacks::PWS_Disable)
+  SV = diag::Severity::Ignored;
+
 // Collect the warning ids.
 SmallVector Ids;
 PP.Lex(Tok);
@@ -1482,6 +1491,14 @@
   }
   Ids.push_back(int(Value));
 }
+if (SV != diag::Severity())
+  for (int Id : Ids)
+

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

[PATCH] D110668: [clang-cl] Accept `#pragma warning(disable : N)` for some N

17 matches

Site Navigation

Mail list logo

Footer information