[PATCH] D51190: [AttrDocs]: document gnu_inline function attribute

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

BTW, seems like these docs have tests that weren't updated. Fixed with 
https://reviews.llvm.org/rL341002, but please take a look if that's not the 
right fix.


Repository:
  rL LLVM

https://reviews.llvm.org/D51190



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

[PATCH] D49724: [VFS] Cleanups to VFS interfaces.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Looks like this caused some test failures 
(http://lab.llvm.org:8011/builders/clang-cmake-aarch64-quick/builds/14441/steps/ninja%20check%201),
 e.g.:

  [ RUN  ] GoToInclude.All
  
/home/buildslave/buildslave/clang-cmake-aarch64-quick/llvm/tools/clang/tools/extra/unittests/clangd/XRefsTests.cpp:953:
 Failure
  Value of: *Locations
  Expected: has 1 element that is equal to 0:0-0:0@file:///clangd-test/foo.h
Actual: {}

I'm not familiar with this patch, but reverting it locally seems to get tests 
passing again, so I'd like to revert this change temporarily.


Repository:
  rL LLVM

https://reviews.llvm.org/D49724



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

[PATCH] D49927: [libc++] Exclude posix_l/strtonum fallback inclusion for newlib > 2.4

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht created this revision.
rupprecht added reviewers: ldionne, rsmith.
Herald added a reviewer: EricWF.
Herald added subscribers: cfe-commits, christof.

[libc++] Exclude posix_l/strtonum fallback inclusion for newlib > 2.4

r338122 changed the linkage of some methods which revealed an existing ODR 
violation, e.g.:
projects/libcxx/include/support/xlocale/__posix_l_fallback.h:83:38: error: 
'internal_linkage' attribute does not appear on the first declaration of 
'iswcntrl_l'
inline _LIBCPP_INLINE_VISIBILITY int iswcntrl_l(wint_t c, locale_t) {

  ^

lib/include/wctype.h:55:12: note: previous definition is here
extern int  iswcntrl_l (wint_t, locale_t);

These were added to newlib in 2.4 [1] [2], so move them to the already existing 
include guard.

[1] 
https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;a=commit;h=238455adfab4f8070ac65400aac22bb8a9e502fc
[2] 
https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;a=commit;h=8493c1631643fada62384768408852bc0fa6ff44


Repository:
  rCXX libc++

https://reviews.llvm.org/D49927

Files:
  include/support/newlib/xlocale.h


Index: include/support/newlib/xlocale.h
===
--- include/support/newlib/xlocale.h
+++ include/support/newlib/xlocale.h
@@ -19,9 +19,9 @@
 #if !defined(__NEWLIB__) || __NEWLIB__ < 2 || \
 __NEWLIB__ == 2 && __NEWLIB_MINOR__ < 5
 #include 
-#endif
 #include 
 #include 
+#endif
 
 #endif // _NEWLIB_VERSION
 


Index: include/support/newlib/xlocale.h
===
--- include/support/newlib/xlocale.h
+++ include/support/newlib/xlocale.h
@@ -19,9 +19,9 @@
 #if !defined(__NEWLIB__) || __NEWLIB__ < 2 || \
 __NEWLIB__ == 2 && __NEWLIB_MINOR__ < 5
 #include 
-#endif
 #include 
 #include 
+#endif
 
 #endif // _NEWLIB_VERSION
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49927: [libc++] Exclude posix_l/strtonum fallback inclusion for newlib > 2.4

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In https://reviews.llvm.org/D49927#1178659, @ldionne wrote:

> Just to make sure I understand properly: this means we will use newlib's 
> implementation of `iswcntrl_l` & friends instead of our own emulation (which 
> is an ODR violation currently going unnoticed)? And this is OK because newlib 
> provides `iswcntrl_l` & friends starting at version 2.4.


Yes, that's correct.
I clarified the description: when the methods were added, the tree was 
configured with NEWLIB_MINOR_VERSION = 4, meaning this wasn't released until 
the next version, newlib 2.5.

> This is my understanding. If it is correct, then LGTM.




Repository:
  rCXX libc++

https://reviews.llvm.org/D49927



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

[PATCH] D49927: [libc++] Exclude posix_l/strtonum fallback inclusion for newlib > 2.4

[Jordan Rupprecht via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL338157: [libc++] Exclude posix_l/strtonum fallback inclusion 
for newlib > 2.4 (authored by rupprecht, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D49927

Files:
  libcxx/trunk/include/support/newlib/xlocale.h


Index: libcxx/trunk/include/support/newlib/xlocale.h
===
--- libcxx/trunk/include/support/newlib/xlocale.h
+++ libcxx/trunk/include/support/newlib/xlocale.h
@@ -19,9 +19,9 @@
 #if !defined(__NEWLIB__) || __NEWLIB__ < 2 || \
 __NEWLIB__ == 2 && __NEWLIB_MINOR__ < 5
 #include 
-#endif
 #include 
 #include 
+#endif
 
 #endif // _NEWLIB_VERSION
 


Index: libcxx/trunk/include/support/newlib/xlocale.h
===
--- libcxx/trunk/include/support/newlib/xlocale.h
+++ libcxx/trunk/include/support/newlib/xlocale.h
@@ -19,9 +19,9 @@
 #if !defined(__NEWLIB__) || __NEWLIB__ < 2 || \
 __NEWLIB__ == 2 && __NEWLIB_MINOR__ < 5
 #include 
-#endif
 #include 
 #include 
+#endif
 
 #endif // _NEWLIB_VERSION
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53066: [Driver] Use forward slashes in most linker arguments

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht resigned from this revision.
rupprecht added a comment.

I'm not familiar with this code or anything windows related, and it looks like 
@rnk is, so I'll remove myself and let him approve


Repository:
  rL LLVM

https://reviews.llvm.org/D53066



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

[PATCH] D56728: Regression test case for r350891 [Correct the source range returned from preprocessor callbacks]

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht created this revision.
rupprecht added a reviewer: aaron.ballman.
Herald added subscribers: cfe-commits, jsji, kbarton, nemanjai.

r350891 caused some internal failures that this test case exposes.


Repository:
  rC Clang

https://reviews.llvm.org/D56728

Files:
  unittests/Lex/PPCallbacksTest.cpp


Index: unittests/Lex/PPCallbacksTest.cpp
===
--- unittests/Lex/PPCallbacksTest.cpp
+++ unittests/Lex/PPCallbacksTest.cpp
@@ -483,6 +483,16 @@
   EXPECT_EQ(
   GetSourceStringToEnd(CharSourceRange(Results7[1].ConditionRange, false)),
   "defined(FLOOFY)");
+  const auto &Results8 =
+  DirectiveExprRange("#define FLOOFY 0\n#if __FILE__ > FLOOFY\n#endif\n");
+  EXPECT_EQ(Results8.size(), 1U);
+  EXPECT_EQ(
+  GetSourceStringToEnd(CharSourceRange(Results8[0].ConditionRange, false)),
+  "__FILE__ > FLOOFY");
+  EXPECT_EQ(
+  Lexer::getSourceText(CharSourceRange(Results8[0].ConditionRange, false),
+   SourceMgr, LangOpts),
+  "__FILE__ > FLOOFY");
 }
 
 } // namespace


Index: unittests/Lex/PPCallbacksTest.cpp
===
--- unittests/Lex/PPCallbacksTest.cpp
+++ unittests/Lex/PPCallbacksTest.cpp
@@ -483,6 +483,16 @@
   EXPECT_EQ(
   GetSourceStringToEnd(CharSourceRange(Results7[1].ConditionRange, false)),
   "defined(FLOOFY)");
+  const auto &Results8 =
+  DirectiveExprRange("#define FLOOFY 0\n#if __FILE__ > FLOOFY\n#endif\n");
+  EXPECT_EQ(Results8.size(), 1U);
+  EXPECT_EQ(
+  GetSourceStringToEnd(CharSourceRange(Results8[0].ConditionRange, false)),
+  "__FILE__ > FLOOFY");
+  EXPECT_EQ(
+  Lexer::getSourceText(CharSourceRange(Results8[0].ConditionRange, false),
+   SourceMgr, LangOpts),
+  "__FILE__ > FLOOFY");
 }
 
 } // namespace
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56728: Regression test case for r350891 [Correct the source range returned from preprocessor callbacks]

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht abandoned this revision.
rupprecht added a comment.

Looks like this test case was submitted as part of rL351209 
.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56728



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

[PATCH] D55964: [clang-format][TableGen] Don't add spaces around items in square braces.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht planned changes to this revision.
rupprecht added a comment.

I'll refactor `getLLVMStyle()` to `getLLVMStyle(FormatStyle::LanguageKind 
Language)` to support this change first.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55964



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

[PATCH] D56943: [clang-format][NFC] Allow getLLVMStyle() to take a language

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

ping


Repository:
  rC Clang

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

https://reviews.llvm.org/D56943



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

[PATCH] D53952: [clang-format] Support breaking consecutive string literals for TableGen

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht created this revision.
rupprecht added a reviewer: krasimir.
Herald added subscribers: cfe-commits, mgorny.
Herald added a reviewer: alexshap.

clang-format can get confused by string literals in TableGen -- e.g. try 
`clang-format tools/llvm-objcopy/ObjcopyOpts.td`; the multiline string literal 
for keep_global_symbols is broken up into tiny little pieces over 15 lines. 
This patch seems to fix that.


Repository:
  rC Clang

https://reviews.llvm.org/D53952

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/CMakeLists.txt
  unittests/Format/FormatTestTableGen.cpp


Index: unittests/Format/FormatTestTableGen.cpp
===
--- /dev/null
+++ unittests/Format/FormatTestTableGen.cpp
@@ -0,0 +1,56 @@
+//===- unittest/Format/FormatTestTableGen.cpp 
-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "FormatTestUtils.h"
+#include "clang/Format/Format.h"
+#include "llvm/Support/Debug.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "format-test"
+
+namespace clang {
+namespace format {
+
+class FormatTestTableGen : public ::testing::Test {
+protected:
+  static std::string format(llvm::StringRef Code, unsigned Offset,
+unsigned Length, const FormatStyle &Style) {
+LLVM_DEBUG(llvm::errs() << "---\n");
+LLVM_DEBUG(llvm::errs() << Code << "\n\n");
+std::vector Ranges(1, tooling::Range(Offset, Length));
+tooling::Replacements Replaces = reformat(Style, Code, Ranges);
+auto Result = applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(Result));
+LLVM_DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
+return *Result;
+  }
+
+  static std::string format(llvm::StringRef Code) {
+FormatStyle Style = getGoogleStyle(FormatStyle::LK_TableGen);
+Style.ColumnLimit = 60; // To make writing tests easier.
+return format(Code, 0, Code.size(), Style);
+  }
+
+  static void verifyFormat(llvm::StringRef Code) {
+EXPECT_EQ(Code.str(), format(Code)) << "Expected code is not stable";
+EXPECT_EQ(Code.str(), format(test::messUp(Code)));
+  }
+};
+
+TEST_F(FormatTestTableGen, FormatStringBreak) {
+  verifyFormat("include \"OptParser.td\"\n"
+   "def flag : Flag<\"--foo\">,\n"
+   "   HelpText<\n"
+   "   \"This is a very, very, very, very, \"\n"
+   "   \"very, very, very, very, very, very, \"\n"
+   "   \"very long help string\">;\n");
+}
+
+} // namespace format
+} // end namespace clang
Index: unittests/Format/CMakeLists.txt
===
--- unittests/Format/CMakeLists.txt
+++ unittests/Format/CMakeLists.txt
@@ -12,6 +12,7 @@
   FormatTestProto.cpp
   FormatTestRawStrings.cpp
   FormatTestSelective.cpp
+  FormatTestTableGen.cpp
   FormatTestTextProto.cpp
   NamespaceEndCommentsFixerTest.cpp
   SortImportsTestJS.cpp
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2874,6 +2874,7 @@
   } else if (Style.Language == FormatStyle::LK_Cpp ||
  Style.Language == FormatStyle::LK_ObjC ||
  Style.Language == FormatStyle::LK_Proto ||
+ Style.Language == FormatStyle::LK_TableGen ||
  Style.Language == FormatStyle::LK_TextProto) {
 if (Left.isStringLiteral() && Right.isStringLiteral())
   return true;


Index: unittests/Format/FormatTestTableGen.cpp
===
--- /dev/null
+++ unittests/Format/FormatTestTableGen.cpp
@@ -0,0 +1,56 @@
+//===- unittest/Format/FormatTestTableGen.cpp -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "FormatTestUtils.h"
+#include "clang/Format/Format.h"
+#include "llvm/Support/Debug.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "format-test"
+
+namespace clang {
+namespace format {
+
+class FormatTestTableGen : public ::testing::Test {
+protected:
+  static std::string format(llvm::StringRef Code, unsigned Offset,
+unsigned Length, const FormatStyle &Style) {
+LLVM_DEBUG(llvm::errs() << "---\n");
+LLVM_DEBUG(llvm::errs() << Code << "\n\n");
+std::vector Ranges(1, tooling::Range(Offset, Length));
+tooling::Replacements Replaces = reformat(Style, Code, Ranges);
+auto Result = applyAllReplacements(Cod

[PATCH] D53952: [clang-format] Support breaking consecutive string literals for TableGen

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

@djasper -- ping


Repository:
  rC Clang

https://reviews.llvm.org/D53952



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

[PATCH] D53952: [clang-format] Support breaking consecutive string literals for TableGen

[Jordan Rupprecht via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC346687: [clang-format] Support breaking consecutive string 
literals for TableGen (authored by rupprecht, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D53952?vs=172029&id=173711#toc

Repository:
  rC Clang

https://reviews.llvm.org/D53952

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/CMakeLists.txt
  unittests/Format/FormatTestTableGen.cpp


Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2875,6 +2875,7 @@
   } else if (Style.Language == FormatStyle::LK_Cpp ||
  Style.Language == FormatStyle::LK_ObjC ||
  Style.Language == FormatStyle::LK_Proto ||
+ Style.Language == FormatStyle::LK_TableGen ||
  Style.Language == FormatStyle::LK_TextProto) {
 if (Left.isStringLiteral() && Right.isStringLiteral())
   return true;
Index: unittests/Format/CMakeLists.txt
===
--- unittests/Format/CMakeLists.txt
+++ unittests/Format/CMakeLists.txt
@@ -12,6 +12,7 @@
   FormatTestProto.cpp
   FormatTestRawStrings.cpp
   FormatTestSelective.cpp
+  FormatTestTableGen.cpp
   FormatTestTextProto.cpp
   NamespaceEndCommentsFixerTest.cpp
   SortImportsTestJS.cpp
Index: unittests/Format/FormatTestTableGen.cpp
===
--- unittests/Format/FormatTestTableGen.cpp
+++ unittests/Format/FormatTestTableGen.cpp
@@ -0,0 +1,56 @@
+//===- unittest/Format/FormatTestTableGen.cpp 
-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "FormatTestUtils.h"
+#include "clang/Format/Format.h"
+#include "llvm/Support/Debug.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "format-test"
+
+namespace clang {
+namespace format {
+
+class FormatTestTableGen : public ::testing::Test {
+protected:
+  static std::string format(llvm::StringRef Code, unsigned Offset,
+unsigned Length, const FormatStyle &Style) {
+LLVM_DEBUG(llvm::errs() << "---\n");
+LLVM_DEBUG(llvm::errs() << Code << "\n\n");
+std::vector Ranges(1, tooling::Range(Offset, Length));
+tooling::Replacements Replaces = reformat(Style, Code, Ranges);
+auto Result = applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(Result));
+LLVM_DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
+return *Result;
+  }
+
+  static std::string format(llvm::StringRef Code) {
+FormatStyle Style = getGoogleStyle(FormatStyle::LK_TableGen);
+Style.ColumnLimit = 60; // To make writing tests easier.
+return format(Code, 0, Code.size(), Style);
+  }
+
+  static void verifyFormat(llvm::StringRef Code) {
+EXPECT_EQ(Code.str(), format(Code)) << "Expected code is not stable";
+EXPECT_EQ(Code.str(), format(test::messUp(Code)));
+  }
+};
+
+TEST_F(FormatTestTableGen, FormatStringBreak) {
+  verifyFormat("include \"OptParser.td\"\n"
+   "def flag : Flag<\"--foo\">,\n"
+   "   HelpText<\n"
+   "   \"This is a very, very, very, very, \"\n"
+   "   \"very, very, very, very, very, very, \"\n"
+   "   \"very long help string\">;\n");
+}
+
+} // namespace format
+} // end namespace clang


Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2875,6 +2875,7 @@
   } else if (Style.Language == FormatStyle::LK_Cpp ||
  Style.Language == FormatStyle::LK_ObjC ||
  Style.Language == FormatStyle::LK_Proto ||
+ Style.Language == FormatStyle::LK_TableGen ||
  Style.Language == FormatStyle::LK_TextProto) {
 if (Left.isStringLiteral() && Right.isStringLiteral())
   return true;
Index: unittests/Format/CMakeLists.txt
===
--- unittests/Format/CMakeLists.txt
+++ unittests/Format/CMakeLists.txt
@@ -12,6 +12,7 @@
   FormatTestProto.cpp
   FormatTestRawStrings.cpp
   FormatTestSelective.cpp
+  FormatTestTableGen.cpp
   FormatTestTextProto.cpp
   NamespaceEndCommentsFixerTest.cpp
   SortImportsTestJS.cpp
Index: unittests/Format/FormatTestTableGen.cpp
===
--- unittests/Format/FormatTestTableGen.cpp
+++ unittests/Format/FormatTestTableGen.cpp
@@ -0,0 +1,56 @@
+//===- unittest/Format/FormatTestTableGen.cpp -===//
+//
+// The LLVM Compile

[PATCH] D56943: [clang-format][NFC] Allow getLLVMStyle() to take a language

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht updated this revision to Diff 188584.
rupprecht marked 2 inline comments as done.
rupprecht added a comment.

Revert getLLVMStyle(LK_Cpp) fixes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56943

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


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -120,6 +120,15 @@
   EXPECT_EQ("a\n#b c d\ne", test::messUp("a\n#b\\\nc\\\nd\ne"));
 }
 
+TEST_F(FormatTest, DefaultLLVMStyleIsCpp) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, getLLVMStyle().Language);
+}
+
+TEST_F(FormatTest, LLVMStyleOverride) {
+  EXPECT_EQ(FormatStyle::LK_Proto,
+getLLVMStyle(FormatStyle::LK_Proto).Language);
+}
+
 
//===--===//
 // Basic function tests.
 
//===--===//
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -618,9 +618,9 @@
   return Expanded;
 }
 
-FormatStyle getLLVMStyle() {
+FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   FormatStyle LLVMStyle;
-  LLVMStyle.Language = FormatStyle::LK_Cpp;
+  LLVMStyle.Language = Language;
   LLVMStyle.AccessModifierOffset = -2;
   LLVMStyle.AlignEscapedNewlines = FormatStyle::ENAS_Right;
   LLVMStyle.AlignAfterOpenBracket = FormatStyle::BAS_Align;
@@ -729,8 +729,7 @@
 return GoogleStyle;
   }
 
-  FormatStyle GoogleStyle = getLLVMStyle();
-  GoogleStyle.Language = Language;
+  FormatStyle GoogleStyle = getLLVMStyle(Language);
 
   GoogleStyle.AccessModifierOffset = -1;
   GoogleStyle.AlignEscapedNewlines = FormatStyle::ENAS_Left;
@@ -2344,8 +2343,7 @@
   if (!FS) {
 FS = llvm::vfs::getRealFileSystem().get();
   }
-  FormatStyle Style = getLLVMStyle();
-  Style.Language = guessLanguage(FileName, Code);
+  FormatStyle Style = getLLVMStyle(guessLanguage(FileName, Code));
 
   FormatStyle FallbackStyle = getNoStyle();
   if (!getPredefinedStyle(FallbackStyleName, Style.Language, &FallbackStyle))
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -1849,7 +1849,8 @@
 
 /// Returns a format style complying with the LLVM coding standards:
 /// http://llvm.org/docs/CodingStandards.html.
-FormatStyle getLLVMStyle();
+FormatStyle getLLVMStyle(
+FormatStyle::LanguageKind Language = FormatStyle::LanguageKind::LK_Cpp);
 
 /// Returns a format style complying with one of Google's style guides:
 /// http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml.


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -120,6 +120,15 @@
   EXPECT_EQ("a\n#b c d\ne", test::messUp("a\n#b\\\nc\\\nd\ne"));
 }
 
+TEST_F(FormatTest, DefaultLLVMStyleIsCpp) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, getLLVMStyle().Language);
+}
+
+TEST_F(FormatTest, LLVMStyleOverride) {
+  EXPECT_EQ(FormatStyle::LK_Proto,
+getLLVMStyle(FormatStyle::LK_Proto).Language);
+}
+
 //===--===//
 // Basic function tests.
 //===--===//
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -618,9 +618,9 @@
   return Expanded;
 }
 
-FormatStyle getLLVMStyle() {
+FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   FormatStyle LLVMStyle;
-  LLVMStyle.Language = FormatStyle::LK_Cpp;
+  LLVMStyle.Language = Language;
   LLVMStyle.AccessModifierOffset = -2;
   LLVMStyle.AlignEscapedNewlines = FormatStyle::ENAS_Right;
   LLVMStyle.AlignAfterOpenBracket = FormatStyle::BAS_Align;
@@ -729,8 +729,7 @@
 return GoogleStyle;
   }
 
-  FormatStyle GoogleStyle = getLLVMStyle();
-  GoogleStyle.Language = Language;
+  FormatStyle GoogleStyle = getLLVMStyle(Language);
 
   GoogleStyle.AccessModifierOffset = -1;
   GoogleStyle.AlignEscapedNewlines = FormatStyle::ENAS_Left;
@@ -2344,8 +2343,7 @@
   if (!FS) {
 FS = llvm::vfs::getRealFileSystem().get();
   }
-  FormatStyle Style = getLLVMStyle();
-  Style.Language = guessLanguage(FileName, Code);
+  FormatStyle Style = getLLVMStyle(guessLanguage(FileName, Code));
 
   FormatStyle FallbackStyle = getNoStyle();
   if (!getPredefinedStyle(FallbackStyleName, Style.Language, &FallbackStyle))
Index: clang/include/clang/Format/F

[PATCH] D56943: [clang-format][NFC] Allow getLLVMStyle() to take a language

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added inline comments.



Comment at: clang/lib/Format/ContinuationIndenter.cpp:193
   RawStringFormat.Language, &PredefinedStyle)) {
-PredefinedStyle = getLLVMStyle();
+PredefinedStyle = getLLVMStyle(FormatStyle::LK_Cpp);
 PredefinedStyle.Language = RawStringFormat.Language;

MyDeveloperDay wrote:
> if you've set up a default argument of LK_Cpp do you need to keep supplying 
> it? if your going to supply it everywhere don't make it a default argument or 
> vice versa
> 
> This might make a much smaller patch, a little more digestable and all the 
> tests can remain as they are.
My goal was to fix all the in-tree callers but leave it as an optional arg (at 
least initially) so as to not break out of tree callers. I reduced it to what I 
actually need for this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56943



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

[PATCH] D56943: [clang-format][NFC] Allow getLLVMStyle() to take a language

[Jordan Rupprecht via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL355123: [clang-format][NFC] Allow getLLVMStyle() to take a 
language (authored by rupprecht, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56943?vs=188584&id=188764#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56943

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


Index: cfe/trunk/include/clang/Format/Format.h
===
--- cfe/trunk/include/clang/Format/Format.h
+++ cfe/trunk/include/clang/Format/Format.h
@@ -1849,7 +1849,8 @@
 
 /// Returns a format style complying with the LLVM coding standards:
 /// http://llvm.org/docs/CodingStandards.html.
-FormatStyle getLLVMStyle();
+FormatStyle getLLVMStyle(
+FormatStyle::LanguageKind Language = FormatStyle::LanguageKind::LK_Cpp);
 
 /// Returns a format style complying with one of Google's style guides:
 /// http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml.
Index: cfe/trunk/lib/Format/Format.cpp
===
--- cfe/trunk/lib/Format/Format.cpp
+++ cfe/trunk/lib/Format/Format.cpp
@@ -618,9 +618,9 @@
   return Expanded;
 }
 
-FormatStyle getLLVMStyle() {
+FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   FormatStyle LLVMStyle;
-  LLVMStyle.Language = FormatStyle::LK_Cpp;
+  LLVMStyle.Language = Language;
   LLVMStyle.AccessModifierOffset = -2;
   LLVMStyle.AlignEscapedNewlines = FormatStyle::ENAS_Right;
   LLVMStyle.AlignAfterOpenBracket = FormatStyle::BAS_Align;
@@ -729,8 +729,7 @@
 return GoogleStyle;
   }
 
-  FormatStyle GoogleStyle = getLLVMStyle();
-  GoogleStyle.Language = Language;
+  FormatStyle GoogleStyle = getLLVMStyle(Language);
 
   GoogleStyle.AccessModifierOffset = -1;
   GoogleStyle.AlignEscapedNewlines = FormatStyle::ENAS_Left;
@@ -2344,8 +2343,7 @@
   if (!FS) {
 FS = llvm::vfs::getRealFileSystem().get();
   }
-  FormatStyle Style = getLLVMStyle();
-  Style.Language = guessLanguage(FileName, Code);
+  FormatStyle Style = getLLVMStyle(guessLanguage(FileName, Code));
 
   FormatStyle FallbackStyle = getNoStyle();
   if (!getPredefinedStyle(FallbackStyleName, Style.Language, &FallbackStyle))
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -120,6 +120,15 @@
   EXPECT_EQ("a\n#b c d\ne", test::messUp("a\n#b\\\nc\\\nd\ne"));
 }
 
+TEST_F(FormatTest, DefaultLLVMStyleIsCpp) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, getLLVMStyle().Language);
+}
+
+TEST_F(FormatTest, LLVMStyleOverride) {
+  EXPECT_EQ(FormatStyle::LK_Proto,
+getLLVMStyle(FormatStyle::LK_Proto).Language);
+}
+
 
//===--===//
 // Basic function tests.
 
//===--===//


Index: cfe/trunk/include/clang/Format/Format.h
===
--- cfe/trunk/include/clang/Format/Format.h
+++ cfe/trunk/include/clang/Format/Format.h
@@ -1849,7 +1849,8 @@
 
 /// Returns a format style complying with the LLVM coding standards:
 /// http://llvm.org/docs/CodingStandards.html.
-FormatStyle getLLVMStyle();
+FormatStyle getLLVMStyle(
+FormatStyle::LanguageKind Language = FormatStyle::LanguageKind::LK_Cpp);
 
 /// Returns a format style complying with one of Google's style guides:
 /// http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml.
Index: cfe/trunk/lib/Format/Format.cpp
===
--- cfe/trunk/lib/Format/Format.cpp
+++ cfe/trunk/lib/Format/Format.cpp
@@ -618,9 +618,9 @@
   return Expanded;
 }
 
-FormatStyle getLLVMStyle() {
+FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   FormatStyle LLVMStyle;
-  LLVMStyle.Language = FormatStyle::LK_Cpp;
+  LLVMStyle.Language = Language;
   LLVMStyle.AccessModifierOffset = -2;
   LLVMStyle.AlignEscapedNewlines = FormatStyle::ENAS_Right;
   LLVMStyle.AlignAfterOpenBracket = FormatStyle::BAS_Align;
@@ -729,8 +729,7 @@
 return GoogleStyle;
   }
 
-  FormatStyle GoogleStyle = getLLVMStyle();
-  GoogleStyle.Language = Language;
+  FormatStyle GoogleStyle = getLLVMStyle(Language);
 
   GoogleStyle.AccessModifierOffset = -1;
   GoogleStyle.AlignEscapedNewlines = FormatStyle::ENAS_Left;
@@ -2344,8 +2343,7 @@
   if (!FS) {
 FS = llvm::vfs::getRealFileSystem().get();
   }
-  FormatStyle Style = getLLVMStyle();
-  Style.Language = guessLanguage(FileName, Code);
+  FormatStyle Style = getLLVMStyle(guessLanguage(FileName, Code));
 
   FormatStyle Fallba

[PATCH] D55964: [clang-format][TableGen] Don't add spaces around items in square braces.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht updated this revision to Diff 188765.
rupprecht added a comment.

Rebase after NFC commit rC355123 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D55964

Files:
  clang/lib/Format/Format.cpp
  clang/unittests/Format/FormatTestTableGen.cpp


Index: clang/unittests/Format/FormatTestTableGen.cpp
===
--- clang/unittests/Format/FormatTestTableGen.cpp
+++ clang/unittests/Format/FormatTestTableGen.cpp
@@ -51,5 +51,9 @@
"   \"very long help string\">;\n");
 }
 
+TEST_F(FormatTestTableGen, NoSpacesInSquareBracketLists) {
+  verifyFormat("def flag : Flag<[\"-\", \"--\"], \"foo\">;\n");
+}
+
 } // namespace format
 } // end namespace clang
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -718,6 +718,11 @@
   LLVMStyle.StatementMacros.push_back("Q_UNUSED");
   LLVMStyle.StatementMacros.push_back("QT_REQUIRE_VERSION");
 
+  // Defaults that differ when not C++.
+  if (Language == FormatStyle::LK_TableGen) {
+LLVMStyle.SpacesInContainerLiterals = false;
+  }
+
   return LLVMStyle;
 }
 


Index: clang/unittests/Format/FormatTestTableGen.cpp
===
--- clang/unittests/Format/FormatTestTableGen.cpp
+++ clang/unittests/Format/FormatTestTableGen.cpp
@@ -51,5 +51,9 @@
"   \"very long help string\">;\n");
 }
 
+TEST_F(FormatTestTableGen, NoSpacesInSquareBracketLists) {
+  verifyFormat("def flag : Flag<[\"-\", \"--\"], \"foo\">;\n");
+}
+
 } // namespace format
 } // end namespace clang
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -718,6 +718,11 @@
   LLVMStyle.StatementMacros.push_back("Q_UNUSED");
   LLVMStyle.StatementMacros.push_back("QT_REQUIRE_VERSION");
 
+  // Defaults that differ when not C++.
+  if (Language == FormatStyle::LK_TableGen) {
+LLVMStyle.SpacesInContainerLiterals = false;
+  }
+
   return LLVMStyle;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55964: [clang-format][TableGen] Don't add spaces around items in square braces.

[Jordan Rupprecht via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC355158: [clang-format][TableGen] Don't add spaces 
around items in square braces. (authored by rupprecht, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D55964?vs=188765&id=188807#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D55964

Files:
  lib/Format/Format.cpp
  unittests/Format/FormatTestTableGen.cpp


Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -718,6 +718,11 @@
   LLVMStyle.StatementMacros.push_back("Q_UNUSED");
   LLVMStyle.StatementMacros.push_back("QT_REQUIRE_VERSION");
 
+  // Defaults that differ when not C++.
+  if (Language == FormatStyle::LK_TableGen) {
+LLVMStyle.SpacesInContainerLiterals = false;
+  }
+
   return LLVMStyle;
 }
 
Index: unittests/Format/FormatTestTableGen.cpp
===
--- unittests/Format/FormatTestTableGen.cpp
+++ unittests/Format/FormatTestTableGen.cpp
@@ -51,5 +51,9 @@
"   \"very long help string\">;\n");
 }
 
+TEST_F(FormatTestTableGen, NoSpacesInSquareBracketLists) {
+  verifyFormat("def flag : Flag<[\"-\", \"--\"], \"foo\">;\n");
+}
+
 } // namespace format
 } // end namespace clang


Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -718,6 +718,11 @@
   LLVMStyle.StatementMacros.push_back("Q_UNUSED");
   LLVMStyle.StatementMacros.push_back("QT_REQUIRE_VERSION");
 
+  // Defaults that differ when not C++.
+  if (Language == FormatStyle::LK_TableGen) {
+LLVMStyle.SpacesInContainerLiterals = false;
+  }
+
   return LLVMStyle;
 }
 
Index: unittests/Format/FormatTestTableGen.cpp
===
--- unittests/Format/FormatTestTableGen.cpp
+++ unittests/Format/FormatTestTableGen.cpp
@@ -51,5 +51,9 @@
"   \"very long help string\">;\n");
 }
 
+TEST_F(FormatTestTableGen, NoSpacesInSquareBracketLists) {
+  verifyFormat("def flag : Flag<[\"-\", \"--\"], \"foo\">;\n");
+}
+
 } // namespace format
 } // end namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59557: Fix CodeGen/arm64-microsoft-status-reg.cpp test

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht created this revision.
rupprecht added reviewers: arsenm, MatzeB.
Herald added subscribers: cfe-commits, kristof.beyls, javed.absar, wdng.
Herald added a project: clang.

This test is failing after r356499. Update the register selection used in the 
test.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D59557

Files:
  clang/test/CodeGen/arm64-microsoft-status-reg.cpp

Index: clang/test/CodeGen/arm64-microsoft-status-reg.cpp
===
--- clang/test/CodeGen/arm64-microsoft-status-reg.cpp
+++ clang/test/CodeGen/arm64-microsoft-status-reg.cpp
@@ -30,103 +30,103 @@
 void check_ReadWriteStatusReg(__int64 v) {
   __int64 ret;
   ret = _ReadStatusReg(ARM64_CNTVCT);
-// CHECK-ASM: mrs x0, CNTVCT_EL0
+// CHECK-ASM: mrs x8, CNTVCT_EL0
 // CHECK-IR: %[[VAR:.*]] = call i64 @llvm.read_register.i64(metadata ![[MD2:.*]])
 // CHECK-IR-NEXT: store i64 %[[VAR]]
 
   ret = _ReadStatusReg(ARM64_PMCCNTR_EL0);
-// CHECK-ASM: mrs x0, PMCCNTR_EL0
+// CHECK-ASM: mrs x8, PMCCNTR_EL0
 // CHECK-IR: %[[VAR:.*]] = call i64 @llvm.read_register.i64(metadata ![[MD3:.*]])
 // CHECK-IR-NEXT: store i64 %[[VAR]]
 
   ret = _ReadStatusReg(ARM64_PMSELR_EL0);
-// CHECK-ASM: mrs x0, PMSELR_EL0
+// CHECK-ASM: mrs x8, PMSELR_EL0
 // CHECK-IR: %[[VAR:.*]] = call i64 @llvm.read_register.i64(metadata ![[MD4:.*]])
 // CHECK-IR-NEXT: store i64 %[[VAR]]
 
   ret = _ReadStatusReg(ARM64_PMXEVCNTR_EL0);
-// CHECK-ASM: mrs x0, PMXEVCNTR_EL0
+// CHECK-ASM: mrs x8, PMXEVCNTR_EL0
 // CHECK-IR: %[[VAR:.*]] = call i64 @llvm.read_register.i64(metadata ![[MD5:.*]])
 // CHECK-IR-NEXT: store i64 %[[VAR]]
 
   ret = _ReadStatusReg(ARM64_PMXEVCNTRn_EL0(0));
-// CHECK-ASM: mrs x0, PMEVCNTR0_EL0
+// CHECK-ASM: mrs x8, PMEVCNTR0_EL0
 // CHECK-IR: %[[VAR:.*]] = call i64 @llvm.read_register.i64(metadata ![[MD6:.*]])
 // CHECK-IR-NEXT: store i64 %[[VAR]]
 
   ret = _ReadStatusReg(ARM64_PMXEVCNTRn_EL0(1));
-// CHECK-ASM: mrs x0, PMEVCNTR1_EL0
+// CHECK-ASM: mrs x8, PMEVCNTR1_EL0
 // CHECK-IR: %[[VAR:.*]] = call i64 @llvm.read_register.i64(metadata ![[MD7:.*]])
 // CHECK-IR-NEXT: store i64 %[[VAR]]
 
   ret = _ReadStatusReg(ARM64_PMXEVCNTRn_EL0(30));
-// CHECK-ASM: mrs x0, PMEVCNTR30_EL0
+// CHECK-ASM: mrs x8, PMEVCNTR30_EL0
 // CHECK-IR: %[[VAR:.*]] = call i64 @llvm.read_register.i64(metadata ![[MD8:.*]])
 // CHECK-IR-NEXT: store i64 %[[VAR]]
 
   ret = _ReadStatusReg(ARM64_TPIDR_EL0);
-// CHECK-ASM: mrs x0, TPIDR_EL0
+// CHECK-ASM: mrs x8, TPIDR_EL0
 // CHECK-IR: %[[VAR:.*]] = call i64 @llvm.read_register.i64(metadata ![[MD9:.*]])
 // CHECK-IR-NEXT: store i64 %[[VAR]]
 
   ret = _ReadStatusReg(ARM64_TPIDRRO_EL0);
-// CHECK-ASM: mrs x0, TPIDRRO_EL0
+// CHECK-ASM: mrs x8, TPIDRRO_EL0
 // CHECK-IR: %[[VAR:.*]] = call i64 @llvm.read_register.i64(metadata ![[MD10:.*]])
 // CHECK-IR-NEXT: store i64 %[[VAR]]
 
   ret = _ReadStatusReg(ARM64_TPIDR_EL1);
-// CHECK-ASM: mrs x0, TPIDR_EL1
+// CHECK-ASM: mrs x8, TPIDR_EL1
 // CHECK-IR: %[[VAR:.*]] = call i64 @llvm.read_register.i64(metadata ![[MD11:.*]])
 // CHECK-IR-NEXT: store i64 %[[VAR]]
 
 
   _WriteStatusReg(ARM64_CNTVCT, v);
-// CHECK-ASM: msr S3_3_C14_C0_2, x0
+// CHECK-ASM: msr S3_3_C14_C0_2, x8
 // CHECK-IR: %[[VAR:.*]] = load i64,
 // CHECK-IR-NEXT: call void @llvm.write_register.i64(metadata ![[MD2:.*]], i64 %[[VAR]])
 
   _WriteStatusReg(ARM64_PMCCNTR_EL0, v);
-// CHECK-ASM: msr PMCCNTR_EL0, x0
+// CHECK-ASM: msr PMCCNTR_EL0, x8
 // CHECK-IR: %[[VAR:.*]] = load i64,
 // CHECK-IR-NEXT: call void @llvm.write_register.i64(metadata ![[MD3:.*]], i64 %[[VAR]])
 
   _WriteStatusReg(ARM64_PMSELR_EL0, v);
-// CHECK-ASM: msr PMSELR_EL0, x0
+// CHECK-ASM: msr PMSELR_EL0, x8
 // CHECK-IR: %[[VAR:.*]] = load i64,
 // CHECK-IR-NEXT: call void @llvm.write_register.i64(metadata ![[MD4:.*]], i64 %[[VAR]])
 
   _WriteStatusReg(ARM64_PMXEVCNTR_EL0, v);
-// CHECK-ASM: msr PMXEVCNTR_EL0, x0
+// CHECK-ASM: msr PMXEVCNTR_EL0, x8
 // CHECK-IR: %[[VAR:.*]] = load i64,
 // CHECK-IR-NEXT: call void @llvm.write_register.i64(metadata ![[MD5:.*]], i64 %[[VAR]])
 
   _WriteStatusReg(ARM64_PMXEVCNTRn_EL0(0), v);
-// CHECK-ASM: msr PMEVCNTR0_EL0, x0
+// CHECK-ASM: msr PMEVCNTR0_EL0, x8
 // CHECK-IR: %[[VAR:.*]] = load i64,
 // CHECK-IR-NEXT: call void @llvm.write_register.i64(metadata ![[MD6:.*]], i64 %[[VAR]])
 
   _WriteStatusReg(ARM64_PMXEVCNTRn_EL0(1), v);
-// CHECK-ASM: msr PMEVCNTR1_EL0, x0
+// CHECK-ASM: msr PMEVCNTR1_EL0, x8
 // CHECK-IR: %[[VAR:.*]] = load i64,
 // CHECK-IR-NEXT: call void @llvm.write_register.i64(metadata ![[MD7:.*]], i64 %[[VAR]])
 
   _WriteStatusReg(ARM64_PMXEVCNTRn_EL0(30), v);
-// CHECK-ASM: msr PMEVCNTR30_EL0, x0
+// CHECK-ASM: msr PMEVCNTR30_EL0, x8
 // CHECK-IR: %[[VAR:.*]] = load i64,
 // CHECK-IR-NEXT: call void @llvm.write_register.i64(metadata ![[MD8:.*]], i64 %[[VAR]])
 
   _WriteStatusReg(ARM64_TPI

[PATCH] D59557: Fix CodeGen/arm64-microsoft-status-reg.cpp test

[Jordan Rupprecht via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC356517: Fix CodeGen/arm64-microsoft-status-reg.cpp test 
(authored by rupprecht, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D59557?vs=191379&id=191390#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D59557

Files:
  test/CodeGen/arm64-microsoft-status-reg.cpp

Index: test/CodeGen/arm64-microsoft-status-reg.cpp
===
--- test/CodeGen/arm64-microsoft-status-reg.cpp
+++ test/CodeGen/arm64-microsoft-status-reg.cpp
@@ -30,103 +30,103 @@
 void check_ReadWriteStatusReg(__int64 v) {
   __int64 ret;
   ret = _ReadStatusReg(ARM64_CNTVCT);
-// CHECK-ASM: mrs x0, CNTVCT_EL0
+// CHECK-ASM: mrs x8, CNTVCT_EL0
 // CHECK-IR: %[[VAR:.*]] = call i64 @llvm.read_register.i64(metadata ![[MD2:.*]])
 // CHECK-IR-NEXT: store i64 %[[VAR]]
 
   ret = _ReadStatusReg(ARM64_PMCCNTR_EL0);
-// CHECK-ASM: mrs x0, PMCCNTR_EL0
+// CHECK-ASM: mrs x8, PMCCNTR_EL0
 // CHECK-IR: %[[VAR:.*]] = call i64 @llvm.read_register.i64(metadata ![[MD3:.*]])
 // CHECK-IR-NEXT: store i64 %[[VAR]]
 
   ret = _ReadStatusReg(ARM64_PMSELR_EL0);
-// CHECK-ASM: mrs x0, PMSELR_EL0
+// CHECK-ASM: mrs x8, PMSELR_EL0
 // CHECK-IR: %[[VAR:.*]] = call i64 @llvm.read_register.i64(metadata ![[MD4:.*]])
 // CHECK-IR-NEXT: store i64 %[[VAR]]
 
   ret = _ReadStatusReg(ARM64_PMXEVCNTR_EL0);
-// CHECK-ASM: mrs x0, PMXEVCNTR_EL0
+// CHECK-ASM: mrs x8, PMXEVCNTR_EL0
 // CHECK-IR: %[[VAR:.*]] = call i64 @llvm.read_register.i64(metadata ![[MD5:.*]])
 // CHECK-IR-NEXT: store i64 %[[VAR]]
 
   ret = _ReadStatusReg(ARM64_PMXEVCNTRn_EL0(0));
-// CHECK-ASM: mrs x0, PMEVCNTR0_EL0
+// CHECK-ASM: mrs x8, PMEVCNTR0_EL0
 // CHECK-IR: %[[VAR:.*]] = call i64 @llvm.read_register.i64(metadata ![[MD6:.*]])
 // CHECK-IR-NEXT: store i64 %[[VAR]]
 
   ret = _ReadStatusReg(ARM64_PMXEVCNTRn_EL0(1));
-// CHECK-ASM: mrs x0, PMEVCNTR1_EL0
+// CHECK-ASM: mrs x8, PMEVCNTR1_EL0
 // CHECK-IR: %[[VAR:.*]] = call i64 @llvm.read_register.i64(metadata ![[MD7:.*]])
 // CHECK-IR-NEXT: store i64 %[[VAR]]
 
   ret = _ReadStatusReg(ARM64_PMXEVCNTRn_EL0(30));
-// CHECK-ASM: mrs x0, PMEVCNTR30_EL0
+// CHECK-ASM: mrs x8, PMEVCNTR30_EL0
 // CHECK-IR: %[[VAR:.*]] = call i64 @llvm.read_register.i64(metadata ![[MD8:.*]])
 // CHECK-IR-NEXT: store i64 %[[VAR]]
 
   ret = _ReadStatusReg(ARM64_TPIDR_EL0);
-// CHECK-ASM: mrs x0, TPIDR_EL0
+// CHECK-ASM: mrs x8, TPIDR_EL0
 // CHECK-IR: %[[VAR:.*]] = call i64 @llvm.read_register.i64(metadata ![[MD9:.*]])
 // CHECK-IR-NEXT: store i64 %[[VAR]]
 
   ret = _ReadStatusReg(ARM64_TPIDRRO_EL0);
-// CHECK-ASM: mrs x0, TPIDRRO_EL0
+// CHECK-ASM: mrs x8, TPIDRRO_EL0
 // CHECK-IR: %[[VAR:.*]] = call i64 @llvm.read_register.i64(metadata ![[MD10:.*]])
 // CHECK-IR-NEXT: store i64 %[[VAR]]
 
   ret = _ReadStatusReg(ARM64_TPIDR_EL1);
-// CHECK-ASM: mrs x0, TPIDR_EL1
+// CHECK-ASM: mrs x8, TPIDR_EL1
 // CHECK-IR: %[[VAR:.*]] = call i64 @llvm.read_register.i64(metadata ![[MD11:.*]])
 // CHECK-IR-NEXT: store i64 %[[VAR]]
 
 
   _WriteStatusReg(ARM64_CNTVCT, v);
-// CHECK-ASM: msr S3_3_C14_C0_2, x0
+// CHECK-ASM: msr S3_3_C14_C0_2, x8
 // CHECK-IR: %[[VAR:.*]] = load i64,
 // CHECK-IR-NEXT: call void @llvm.write_register.i64(metadata ![[MD2:.*]], i64 %[[VAR]])
 
   _WriteStatusReg(ARM64_PMCCNTR_EL0, v);
-// CHECK-ASM: msr PMCCNTR_EL0, x0
+// CHECK-ASM: msr PMCCNTR_EL0, x8
 // CHECK-IR: %[[VAR:.*]] = load i64,
 // CHECK-IR-NEXT: call void @llvm.write_register.i64(metadata ![[MD3:.*]], i64 %[[VAR]])
 
   _WriteStatusReg(ARM64_PMSELR_EL0, v);
-// CHECK-ASM: msr PMSELR_EL0, x0
+// CHECK-ASM: msr PMSELR_EL0, x8
 // CHECK-IR: %[[VAR:.*]] = load i64,
 // CHECK-IR-NEXT: call void @llvm.write_register.i64(metadata ![[MD4:.*]], i64 %[[VAR]])
 
   _WriteStatusReg(ARM64_PMXEVCNTR_EL0, v);
-// CHECK-ASM: msr PMXEVCNTR_EL0, x0
+// CHECK-ASM: msr PMXEVCNTR_EL0, x8
 // CHECK-IR: %[[VAR:.*]] = load i64,
 // CHECK-IR-NEXT: call void @llvm.write_register.i64(metadata ![[MD5:.*]], i64 %[[VAR]])
 
   _WriteStatusReg(ARM64_PMXEVCNTRn_EL0(0), v);
-// CHECK-ASM: msr PMEVCNTR0_EL0, x0
+// CHECK-ASM: msr PMEVCNTR0_EL0, x8
 // CHECK-IR: %[[VAR:.*]] = load i64,
 // CHECK-IR-NEXT: call void @llvm.write_register.i64(metadata ![[MD6:.*]], i64 %[[VAR]])
 
   _WriteStatusReg(ARM64_PMXEVCNTRn_EL0(1), v);
-// CHECK-ASM: msr PMEVCNTR1_EL0, x0
+// CHECK-ASM: msr PMEVCNTR1_EL0, x8
 // CHECK-IR: %[[VAR:.*]] = load i64,
 // CHECK-IR-NEXT: call void @llvm.write_register.i64(metadata ![[MD7:.*]], i64 %[[VAR]])
 
   _WriteStatusReg(ARM64_PMXEVCNTRn_EL0(30), v);
-// CHECK-ASM: msr PMEVCNTR30_EL0, x0
+// CHECK-ASM: msr PMEVCNTR30_EL0, x8
 // CHECK-IR: %[[VAR:.*]] = load i64,
 // CHECK-IR-NEXT: call void @llvm.write_register.i64(metadata ![[MD8:.*]], i64 %[[VAR

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added inline comments.



Comment at: cfe/trunk/unittests/AST/OMPStructuredBlockTest.cpp:58
+  std::vector Args = {
+  "-fopenmp",
+  };

Looks like this test fails when the default is not libomp, e.g. 
DCLANG_DEFAULT_OPENMP_RUNTIME=libgomp
Using -fopenmp=libomp explicitly here causes the test to pass in the case. Does 
that seem like the right fix, or has something gone wrong?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59214



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

[PATCH] D59609: [clang][OpenMP] Fix build when using libgomp

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht created this revision.
rupprecht added a reviewer: lebedev.ri.
Herald added subscribers: cfe-commits, jdoerfert, guansong.
Herald added a project: clang.

rL356570  introduced a test which only 
passes with the default openmp library, libomp, and fails with other openmp 
libraries, such as libgomp. Explicitly choose libomp.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D59609

Files:
  clang/unittests/AST/OMPStructuredBlockTest.cpp


Index: clang/unittests/AST/OMPStructuredBlockTest.cpp
===
--- clang/unittests/AST/OMPStructuredBlockTest.cpp
+++ clang/unittests/AST/OMPStructuredBlockTest.cpp
@@ -55,7 +55,7 @@
   StringRef ExpectedPrinted,
   PolicyAdjusterType PolicyAdjuster = None) {
   std::vector Args = {
-  "-fopenmp",
+  "-fopenmp=libomp",
   };
   return PrintedStmtMatches(Code, Args, NodeMatch, ExpectedPrinted,
 PolicyAdjuster);


Index: clang/unittests/AST/OMPStructuredBlockTest.cpp
===
--- clang/unittests/AST/OMPStructuredBlockTest.cpp
+++ clang/unittests/AST/OMPStructuredBlockTest.cpp
@@ -55,7 +55,7 @@
   StringRef ExpectedPrinted,
   PolicyAdjusterType PolicyAdjuster = None) {
   std::vector Args = {
-  "-fopenmp",
+  "-fopenmp=libomp",
   };
   return PrintedStmtMatches(Code, Args, NodeMatch, ExpectedPrinted,
 PolicyAdjuster);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59609: [clang][OpenMP] Fix build when using libgomp

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D59609#1436975 , @lebedev.ri wrote:

> Interesting.
>  What happens if libomp is not being built?
>  What happens if libomp is not present?


I'm far from an omp expert (only running across this because some tests 
failed), but I would guess both those cases to break.

But this is broken anyway when libomp isn't being used. So the set of broken 
cases is going from (people that don't use libomp) to (people that don't have 
libomp) which I think is a much smaller subset.

> Or more generally, why does it even matter whether there is runtime or not, 
> this only does paring+sema, no codegen/execution.

I'm very surprised myself. Hence my question "has something gone wrong" on 
D59214 . Hopefully this fix/hack can be 
reverted once that's figured out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59609



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

[PATCH] D59609: [clang][OpenMP] Fix build when using libgomp

[Jordan Rupprecht via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC356614: [clang][OpenMP] Fix build when using libgomp 
(authored by rupprecht, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D59609?vs=191567&id=191577#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D59609

Files:
  unittests/AST/OMPStructuredBlockTest.cpp


Index: unittests/AST/OMPStructuredBlockTest.cpp
===
--- unittests/AST/OMPStructuredBlockTest.cpp
+++ unittests/AST/OMPStructuredBlockTest.cpp
@@ -55,7 +55,7 @@
   StringRef ExpectedPrinted,
   PolicyAdjusterType PolicyAdjuster = None) {
   std::vector Args = {
-  "-fopenmp",
+  "-fopenmp=libomp",
   };
   return PrintedStmtMatches(Code, Args, NodeMatch, ExpectedPrinted,
 PolicyAdjuster);


Index: unittests/AST/OMPStructuredBlockTest.cpp
===
--- unittests/AST/OMPStructuredBlockTest.cpp
+++ unittests/AST/OMPStructuredBlockTest.cpp
@@ -55,7 +55,7 @@
   StringRef ExpectedPrinted,
   PolicyAdjusterType PolicyAdjuster = None) {
   std::vector Args = {
-  "-fopenmp",
+  "-fopenmp=libomp",
   };
   return PrintedStmtMatches(Code, Args, NodeMatch, ExpectedPrinted,
 PolicyAdjuster);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60974: Clang IFSO driver action.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

I didn't follow the technical details, but I don't see anything wrong with 
moving forward on this patch. I think this seems like an interesting idea worth 
experimenting with.

In D60974#1488563 , @jakehehrlich 
wrote:

> > Jake, I am still not sure what you would prefer for moving forward. The 
> > current patch can output the yaml format I originally proposed or the one 
> > that looks similar to the one you proposed. What I need to know is:
> > 
> > Do you want the merging if the "ifo" files to happen inside of llvm-elfabi?
> >  Do you care if we upstream the yaml we proposed as a first step (which is 
> > really a distilled version of that yaml2obj can consume anyways. this right 
> > now functions actually) ???
> >  Or, would you rather the ifo files be merged by a different separate tool 
> > (something maybe called llvm-ifsogen)???
>
> This is my proposal:
>
> 1. We should plan on adding the code ofr merging these files inside of 
> `llvm/tools/llvm-elfabi` but will it be a separate tool from `llvm-elfabi`. 
> You can create "separate" tools in the same directory using the symlink 
> trick. See llvm-ar and friends or llvm-objcopy and llvm-strip. The tool name 
> can be discussed in review.


The symlink trick is usually used when the new frontend tool is just a thin 
layer over the underlying tool. Is that going to be the case here?

For example,

- `llvm-ranlib` is equivalent to `llvm-ar s`
- `llvm-readelf` is equivalent to `llvm-readobj --elf-output-style=GNU`
- `llvm-addr2line` is equivalent to `llvm-symbolizer --output-style=GNU`
- `llvm-strip` is equivalent to `llvm-objcopy --strip-all` (maybe not exactly)

In other words, is `llvm-mergeifo` (placeholder name) going to be roughly 
equivalent to `llvm-elfabi --merge-ifo` (again, placeholder flag name)? And if 
so, it doesn't seem like a symlink is strictly necessary -- users can just call 
`llvm-elfabi --merge-ifo`. Am I missing something?

> 2. The yaml proposed thus far is not acceptable IMO for reasons already 
> outlined but this can be discussed in code review. Before adding sections a 
> clear reason for needing them is required which hasn't been given yet. Things 
> can evolve and change later as needed but we should start with the minimum 
> and add as needed later; not start with extra and remove later. Support for 
> the new format should be added into TextAPI and in the review for that 
> process we should discuss the format. After we add support for this new 
> format into TextAPI we can add support for emitting this format into clang 
> (well actually you can write the code whenever but I'm using "after" in a 
> different sense here).

What if we named it experimental for now? i.e. `--experimental-emit-ifo` for 
the clang flag name and `!experimental-ifo-elf-v1` for the yaml id? That would 
allow those working on this patch to play around with more features, but still 
give sufficient warning to anyone that fields they depend on may be removed.

In fact, if we label it experimental (or maybe even if we don't), I don't see 
any reason this couldn't land now, even without a consumer of it. So what if a 
tool produces a yaml file that we don't haven't finished the yaml parser for? 
Does anything break?




Comment at: clang/lib/Driver/Driver.cpp:3449-3450
   return C.MakeAction(Input, types::TY_Nothing);
+if (Args.hasArg(options::OPT_emit_ifso))
+  return C.MakeAction(Input, types::TY_IFO);
 return C.MakeAction(Input, types::TY_LLVM_BC);

bikeshed: I don't think we should have both ifo and ifso. Either name sounds 
fine to me, but if running "-emit-ifso" gives me an ifo and not an ifso, that's 
very surprising, and is going to be a major headache trying to remember when to 
call it ifo and when to call it ifso.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60974



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

[PATCH] D52398: Thread safety analysis: Unwrap __builtin_expect in getTrylockCallExpr

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In https://reviews.llvm.org/D52398#1255290, @aaronpuchert wrote:

> @hokein Please have a look at https://reviews.llvm.org/D52888, maybe you can 
> try it out already. The problem was that `?:` expressions are considered a 
> branch point and when merging both branches the warning was emitted. Before 
> this change, we couldn't look into `__builtin_expect` and so we didn't see 
> that the result of `TryLock` is used to branch.


I patched the proposed fix-forward and it seems to have things building again. 
Is there an ETA on landing that? If it's going to take a bit, is there any 
chance we could revert this patch until then?


Repository:
  rL LLVM

https://reviews.llvm.org/D52398



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

[PATCH] D52398: Thread safety analysis: Unwrap __builtin_expect in getTrylockCallExpr

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In https://reviews.llvm.org/D52398#1257133, @aaronpuchert wrote:

> In https://reviews.llvm.org/D52398#1257092, @rupprecht wrote:
>
> > I patched the proposed fix-forward and it seems to have things building 
> > again. Is there an ETA on landing that? If it's going to take a bit, is 
> > there any chance we could revert this patch until then?
>
>
> The fix was just committed (https://reviews.llvm.org/rC343902). Generally I'm 
> for reverting if there are functional deficiencies, but here it's more that 
> an unrelated issue was accidentally uncovered by this patch.


Ah, I thought it was an issue with this patch. Clearly I shouldn't pretend to 
understand thread analysis :)
Thanks for the fix!


Repository:
  rL LLVM

https://reviews.llvm.org/D52398



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

[PATCH] D56943: [clang-format][NFC] Allow getLLVMStyle() to take a language

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.
Herald added a project: clang.

ping


Repository:
  rC Clang

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

https://reviews.llvm.org/D56943



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

[PATCH] D56943: [clang-format][NFC] Allow getLLVMStyle() to take a language

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D56943#1388314 , @kristina wrote:

> The patch itself looks sound. However given that you have a specific use case 
> in mind (TableGen files) could you provide supplementary coverage for that 
> specific use case (unit tests for formatting `td` syntax using 
> `format::getLLVMStyle(format::FormatStyle::LK_TableGen)`? I'm not entirely 
> sure how useful this particular change is given that there's no linked 
> patches related to your use case, I think adding those would help as well 
> (possibly as a separate dependent patchset).


The use case is a pre-refactoring for D55964 , 
which is in the patch description -- is that what you mean by "linked patches"?
I'll update that patch to use this approach, and make it a dependent patch.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56943



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

[PATCH] D55964: [clang-format][TableGen] Don't add spaces around items in square braces.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht updated this revision to Diff 185787.
rupprecht added a comment.
Herald added a subscriber: arphaman.
Herald added a project: clang.

- Rebased w/ D56943  patched in so we can 
override just TableGen in getLLVMStyle()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D55964

Files:
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Index/CommentToXML.cpp
  clang/unittests/Format/CleanupTest.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestComments.cpp
  clang/unittests/Format/FormatTestObjC.cpp
  clang/unittests/Format/FormatTestRawStrings.cpp
  clang/unittests/Format/FormatTestSelective.cpp
  clang/unittests/Format/FormatTestTableGen.cpp
  clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
  clang/unittests/Format/SortIncludesTest.cpp
  clang/unittests/Format/UsingDeclarationsSorterTest.cpp
  clang/unittests/Rename/ClangRenameTest.h
  clang/unittests/Tooling/HeaderIncludesTest.cpp
  clang/unittests/Tooling/RefactoringTest.cpp

Index: clang/unittests/Tooling/RefactoringTest.cpp
===
--- clang/unittests/Tooling/RefactoringTest.cpp
+++ clang/unittests/Tooling/RefactoringTest.cpp
@@ -1299,7 +1299,7 @@
   ApplyAtomicChangesTest() : FilePath("file.cc") {
 Spec.Cleanup = true;
 Spec.Format = ApplyChangesSpec::kAll;
-Spec.Style = format::getLLVMStyle();
+Spec.Style = format::getLLVMStyle(format::FormatStyle::LK_Cpp);
   }
 
   ~ApplyAtomicChangesTest() override {}
Index: clang/unittests/Tooling/HeaderIncludesTest.cpp
===
--- clang/unittests/Tooling/HeaderIncludesTest.cpp
+++ clang/unittests/Tooling/HeaderIncludesTest.cpp
@@ -41,7 +41,8 @@
   }
 
   const std::string FileName = "fix.cpp";
-  IncludeStyle Style = format::getLLVMStyle().IncludeStyle;
+  IncludeStyle Style =
+  format::getLLVMStyle(format::FormatStyle::LK_Cpp).IncludeStyle;
 };
 
 TEST_F(HeaderIncludesTest, NoExistingIncludeWithoutDefine) {
Index: clang/unittests/Rename/ClangRenameTest.h
===
--- clang/unittests/Rename/ClangRenameTest.h
+++ clang/unittests/Rename/ClangRenameTest.h
@@ -93,8 +93,9 @@
   }
 
   std::string format(llvm::StringRef Code) {
-tooling::Replacements Replaces = format::reformat(
-format::getLLVMStyle(), Code, {tooling::Range(0, Code.size())});
+tooling::Replacements Replaces =
+format::reformat(format::getLLVMStyle(format::FormatStyle::LK_Cpp),
+ Code, {tooling::Range(0, Code.size())});
 auto ChangedCode = tooling::applyAllReplacements(Code, Replaces);
 EXPECT_TRUE(static_cast(ChangedCode));
 if (!ChangedCode) {
Index: clang/unittests/Format/UsingDeclarationsSorterTest.cpp
===
--- clang/unittests/Format/UsingDeclarationsSorterTest.cpp
+++ clang/unittests/Format/UsingDeclarationsSorterTest.cpp
@@ -19,9 +19,9 @@
 
 class UsingDeclarationsSorterTest : public ::testing::Test {
 protected:
-  std::string sortUsingDeclarations(llvm::StringRef Code,
-const std::vector &Ranges,
-const FormatStyle &Style = getLLVMStyle()) {
+  std::string sortUsingDeclarations(
+  llvm::StringRef Code, const std::vector &Ranges,
+  const FormatStyle &Style = getLLVMStyle(FormatStyle::LK_Cpp)) {
 LLVM_DEBUG(llvm::errs() << "---\n");
 LLVM_DEBUG(llvm::errs() << Code << "\n\n");
 tooling::Replacements Replaces =
@@ -32,8 +32,9 @@
 return *Result;
   }
 
-  std::string sortUsingDeclarations(llvm::StringRef Code,
-const FormatStyle &Style = getLLVMStyle()) {
+  std::string sortUsingDeclarations(
+  llvm::StringRef Code,
+  const FormatStyle &Style = getLLVMStyle(FormatStyle::LK_Cpp)) {
 return sortUsingDeclarations(Code,
  /*Ranges=*/{1, tooling::Range(0, Code.size())},
  Style);
Index: clang/unittests/Format/SortIncludesTest.cpp
===
--- clang/unittests/Format/SortIncludesTest.cpp
+++ clang/unittests/Format/SortIncludesTest.cpp
@@ -44,7 +44,7 @@
 return Cursor;
   }
 
-  FormatStyle FmtStyle = getLLVMStyle();
+  FormatStyle FmtStyle = getLLVMStyle(FormatStyle::LK_Cpp);
   tooling::IncludeStyle &Style = FmtStyle.IncludeStyle;
 };
 
Index: clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
===
--- clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
+++ clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
@@ -20,10 +20,9 @@
 
 class NamespaceEndCommentsFix

[PATCH] D56943: [clang-format][NFC] Allow getLLVMStyle() to take a language

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D56943#1388985 , @rupprecht wrote:

> In D56943#1388314 , @kristina wrote:
>
> > The patch itself looks sound. However given that you have a specific use 
> > case in mind (TableGen files) could you provide supplementary coverage for 
> > that specific use case (unit tests for formatting `td` syntax using 
> > `format::getLLVMStyle(format::FormatStyle::LK_TableGen)`? I'm not entirely 
> > sure how useful this particular change is given that there's no linked 
> > patches related to your use case, I think adding those would help as well 
> > (possibly as a separate dependent patchset).
>
>
> The use case is a pre-refactoring for D55964 
> , which is in the patch description -- is 
> that what you mean by "linked patches"?
>  I'll update that patch to use this approach, and make it a dependent patch.


Added as a dependent patch. I've never linked patches together, so I might not 
be doing it right. Guess I'll read some phab docs now...


Repository:
  rC Clang

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

https://reviews.llvm.org/D56943



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

[PATCH] D57896: Variable names rule

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D57896#1391925 , 
@hubert.reinterpretcast wrote:

> In D57896#1391611 , @zturner wrote:
>
> > Is this actually any better?  Whereas before we can’t differentiate type 
> > names and variable names, under this proposal we can’t differentiate type 
> > names and function names.  So it seems a bit of “6 of 1, half dozen of 
> > another”
>
>
> Perhaps you mistyped? The proposal does not change the status quo of either 
> type names nor function names. If you mean that we can't differentiate 
> variable names and function names, then it seems worthwhile to point out that 
> the actual letters (not just the case of said letters) matter too. Whereas 
> the guidelines state that types and variables should have names that are 
> nouns, the guidelines state that functions should have names that are verb 
> phrases.


There is still overlap, e.g. "process" can be a noun (a Linux process) or a 
verb (to process something)

I think it should also be pointed out there is not zero overhead -- it's not a 
lot (at least for native English speakers, which many LLVM developers are not), 
but determining if a word is a verb or a noun is harder than looking at the 
casing. Small, but worth observing.

A different convention, e.g. `lower_case`, avoids this. Personally, I'd prefer 
that, but I'm also fine with lowerCamelCase just so we can stop using 
UpperCamelCase.




Comment at: llvm/docs/CodingStandards.rst:1195
+  be camel case, and start with a lower case letter (e.g. ``leader`` or
+  ``boats``). It is also acceptable to use ``UpperCamelCase`` for consistency
+  with existing code.

It would be nice for this section to be expanded a bit, just to avoid 
inevitable code review churn, e.g. if I'm adding 50 lines to a 200 line file, 
am I allowed to change the existing var names elsewhere in the file or method, 
or is that outside the scope of my change? If I'm reviewing that patch, do I 
tell the author they have to be consistent and revert other changes? etc.

Is there any plan to use clang-tidy to do a global cleanup, or is this going to 
be a totally ad-hoc migration -- variables use the new scheme only when the 
code is updated?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



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

[PATCH] D60974: Clang IFSO driver action.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a reviewer: rupprecht.
rupprecht added a comment.

Can you upload this patch with context? Either use arc or upload w/ -U9

I seem to have a lot of comments, but they're all nits -- my major concern 
being the `llvm_unreachable`s should be errors instead (i.e. should be 
triggered in release builds).

Since this is clearly labeled as experimental, I think you should feel free to 
commit if you can get another lgtm (@compnerd?)




Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3614-3616
+  Args.hasArg(options::OPT_iterface_stub_version_EQ)
+  ? Args.getLastArgValue(options::OPT_iterface_stub_version_EQ)
+  : "")

`Args.getLastArgValue(options::OPT_iterface_stub_version_EQ)` should already 
default to returning the empty string if unset (note the default param)



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1678-1680
+  Args.hasArg(OPT_iterface_stub_version_EQ)
+  ? Args.getLastArgValue(OPT_iterface_stub_version_EQ)
+  : "")

(same here)



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1688
+  if (!ProgramActionPair.second)
+llvm_unreachable("Must specify a valid interface stub format.");
+  Opts.ProgramAction = ProgramActionPair.first;

I think this is very much reachable if someone provides 
`--interface-stub-version=x`



Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:20-21
+  CompilerInstance &Instance;
+  StringRef InFile = "";
+  StringRef Format = "";
+  std::set ParsedTemplates;

nit: these default initializations are redundant



Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:26
+  struct MangledSymbol {
+std::string ParentName = "";
+uint8_t Type;

ditto



Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:39-41
+if (!(RDO & FromTU))
+  return true;
+if (Symbols.find(ND) != Symbols.end())

Can you add a comment why these two are excluded?



Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:41
+  return true;
+if (Symbols.find(ND) != Symbols.end())
+  return true;

llvm::is_contained(Symbols, ND)



Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:44-47
+if (isa(ND))
+  return true;
+if (isa(ND))
+  return true;

This would be terser (and more readable) combined; `if (isa(ND) || 
isa(ND))`



Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:95
+  index::CodegenNameGenerator CGNameGen(ND->getASTContext());
+  auto MangledNames = CGNameGen.getAllManglings(ND);
+  if (MangledNames.size() == 1)

auto -> std::vector, unless the return type is obvious to anyone 
that commits here (i.e. not me)



Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:107
+  index::CodegenNameGenerator CGNameGen(ND->getASTContext());
+  auto MangledName = CGNameGen.getName(ND);
+  auto MangledNames = CGNameGen.getAllManglings(ND);

auto -> std::string



Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:206-207
+// catch anything that's not a VarDecl or Template/FunctionDecl.
+llvm_unreachable("clang -emit-iterface-stubs: Expected a function or "
+ "function template decl.");
+return false;

This should be an error, not llvm_unreachable (which I think gets filtered out 
in release builds)



Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:242
+MangledSymbols Symbols;
+auto OS = Instance.createDefaultOutputFile(/*Binary=*/false, InFile, 
"ifo");
+if (!OS)

elsewhere it looks like this was changed to "ifs"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60974



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

[PATCH] D63625: [CodeGen][test] Use -fno-discard-value-names for better test support

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht created this revision.
rupprecht added reviewers: rnk, akhuang, aprantl.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Depending on how clang is built, it may discard the IR names and use names like 
`%2` instead of `%result.ptr`, causing tests that rely on the IR name to fail. 
Using `fno-discard-value-names` ensures the actual name is present regardless 
of the build mode.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D63625

Files:
  clang/test/CodeGenCXX/debug-info-nrvo.cpp


Index: clang/test/CodeGenCXX/debug-info-nrvo.cpp
===
--- clang/test/CodeGenCXX/debug-info-nrvo.cpp
+++ clang/test/CodeGenCXX/debug-info-nrvo.cpp
@@ -1,5 +1,10 @@
-// RUN: %clangxx -target x86_64-unknown-unknown -g %s -emit-llvm -S -o - | 
FileCheck %s
-// RUN: %clangxx -target x86_64-unknown-unknown -g -fno-elide-constructors %s 
-emit-llvm -S -o - | FileCheck %s -check-prefix=NOELIDE
+// RUN: %clangxx -target x86_64-unknown-unknown -g -fno-discard-value-names \
+// RUN:   %s -emit-llvm -S -o - | FileCheck %s
+
+// RUN: %clangxx -target x86_64-unknown-unknown -g -fno-discard-value-names \
+// RUN:   -fno-elide-constructors %s -emit-llvm -S -o - | \
+// RUN:   FileCheck %s -check-prefix=NOELIDE
+
 struct Foo {
   Foo() = default;
   Foo(Foo &&other) { x = other.x; }


Index: clang/test/CodeGenCXX/debug-info-nrvo.cpp
===
--- clang/test/CodeGenCXX/debug-info-nrvo.cpp
+++ clang/test/CodeGenCXX/debug-info-nrvo.cpp
@@ -1,5 +1,10 @@
-// RUN: %clangxx -target x86_64-unknown-unknown -g %s -emit-llvm -S -o - | FileCheck %s
-// RUN: %clangxx -target x86_64-unknown-unknown -g -fno-elide-constructors %s -emit-llvm -S -o - | FileCheck %s -check-prefix=NOELIDE
+// RUN: %clangxx -target x86_64-unknown-unknown -g -fno-discard-value-names \
+// RUN:   %s -emit-llvm -S -o - | FileCheck %s
+
+// RUN: %clangxx -target x86_64-unknown-unknown -g -fno-discard-value-names \
+// RUN:   -fno-elide-constructors %s -emit-llvm -S -o - | \
+// RUN:   FileCheck %s -check-prefix=NOELIDE
+
 struct Foo {
   Foo() = default;
   Foo(Foo &&other) { x = other.x; }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63625: [CodeGen][test] Use -fno-discard-value-names for better test support

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht updated this revision to Diff 205912.
rupprecht added a comment.

- Use filecheck variable matching instead of an explicit 
-fno-discard-value-names option


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63625

Files:
  clang/test/CodeGenCXX/debug-info-nrvo.cpp


Index: clang/test/CodeGenCXX/debug-info-nrvo.cpp
===
--- clang/test/CodeGenCXX/debug-info-nrvo.cpp
+++ clang/test/CodeGenCXX/debug-info-nrvo.cpp
@@ -1,5 +1,10 @@
-// RUN: %clangxx -target x86_64-unknown-unknown -g %s -emit-llvm -S -o - | 
FileCheck %s
-// RUN: %clangxx -target x86_64-unknown-unknown -g -fno-elide-constructors %s 
-emit-llvm -S -o - | FileCheck %s -check-prefix=NOELIDE
+// RUN: %clangxx -target x86_64-unknown-unknown -g \
+// RUN:   %s -emit-llvm -S -o - | FileCheck %s
+
+// RUN: %clangxx -target x86_64-unknown-unknown -g \
+// RUN:   -fno-elide-constructors %s -emit-llvm -S -o - | \
+// RUN:   FileCheck %s -check-prefix=NOELIDE
+
 struct Foo {
   Foo() = default;
   Foo(Foo &&other) { x = other.x; }
@@ -21,8 +26,10 @@
 // Check that NRVO variables are stored as a pointer with deref if they are
 // stored in the return register.
 
-// CHECK: %result.ptr = alloca i8*, align 8
-// CHECK: call void @llvm.dbg.declare(metadata i8** %result.ptr,
+// CHECK: %[[RESULT:.*]] = alloca i8*, align 8
+// CHECK: call void @llvm.dbg.declare(metadata i8** %[[RESULT]],
 // CHECK-SAME: metadata !DIExpression(DW_OP_deref)
-// NOELIDE: call void @llvm.dbg.declare(metadata %struct.Foo* %foo,
+
+// NOELIDE: %[[FOO:.*]] = alloca %struct.Foo, align 4
+// NOELIDE: call void @llvm.dbg.declare(metadata %struct.Foo* %[[FOO]],
 // NOELIDE-SAME:metadata !DIExpression()


Index: clang/test/CodeGenCXX/debug-info-nrvo.cpp
===
--- clang/test/CodeGenCXX/debug-info-nrvo.cpp
+++ clang/test/CodeGenCXX/debug-info-nrvo.cpp
@@ -1,5 +1,10 @@
-// RUN: %clangxx -target x86_64-unknown-unknown -g %s -emit-llvm -S -o - | FileCheck %s
-// RUN: %clangxx -target x86_64-unknown-unknown -g -fno-elide-constructors %s -emit-llvm -S -o - | FileCheck %s -check-prefix=NOELIDE
+// RUN: %clangxx -target x86_64-unknown-unknown -g \
+// RUN:   %s -emit-llvm -S -o - | FileCheck %s
+
+// RUN: %clangxx -target x86_64-unknown-unknown -g \
+// RUN:   -fno-elide-constructors %s -emit-llvm -S -o - | \
+// RUN:   FileCheck %s -check-prefix=NOELIDE
+
 struct Foo {
   Foo() = default;
   Foo(Foo &&other) { x = other.x; }
@@ -21,8 +26,10 @@
 // Check that NRVO variables are stored as a pointer with deref if they are
 // stored in the return register.
 
-// CHECK: %result.ptr = alloca i8*, align 8
-// CHECK: call void @llvm.dbg.declare(metadata i8** %result.ptr,
+// CHECK: %[[RESULT:.*]] = alloca i8*, align 8
+// CHECK: call void @llvm.dbg.declare(metadata i8** %[[RESULT]],
 // CHECK-SAME: metadata !DIExpression(DW_OP_deref)
-// NOELIDE: call void @llvm.dbg.declare(metadata %struct.Foo* %foo,
+
+// NOELIDE: %[[FOO:.*]] = alloca %struct.Foo, align 4
+// NOELIDE: call void @llvm.dbg.declare(metadata %struct.Foo* %[[FOO]],
 // NOELIDE-SAME:metadata !DIExpression()
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63625: [CodeGen][test] Use -fno-discard-value-names for better test support

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Sounds good, changed to use variable matching instead. This passes w/ either 
`-fno-discard-value-names` or `-fdiscard-value-names` used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63625



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

[PATCH] D63625: [CodeGen][test] Use -fno-discard-value-names for better test support

[Jordan Rupprecht via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG73986707bd57: [CodeGen][test] Use FileCheck variable 
matchers for better test support (authored by rupprecht).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63625

Files:
  clang/test/CodeGenCXX/debug-info-nrvo.cpp


Index: clang/test/CodeGenCXX/debug-info-nrvo.cpp
===
--- clang/test/CodeGenCXX/debug-info-nrvo.cpp
+++ clang/test/CodeGenCXX/debug-info-nrvo.cpp
@@ -1,5 +1,10 @@
-// RUN: %clangxx -target x86_64-unknown-unknown -g %s -emit-llvm -S -o - | 
FileCheck %s
-// RUN: %clangxx -target x86_64-unknown-unknown -g -fno-elide-constructors %s 
-emit-llvm -S -o - | FileCheck %s -check-prefix=NOELIDE
+// RUN: %clangxx -target x86_64-unknown-unknown -g \
+// RUN:   %s -emit-llvm -S -o - | FileCheck %s
+
+// RUN: %clangxx -target x86_64-unknown-unknown -g \
+// RUN:   -fno-elide-constructors %s -emit-llvm -S -o - | \
+// RUN:   FileCheck %s -check-prefix=NOELIDE
+
 struct Foo {
   Foo() = default;
   Foo(Foo &&other) { x = other.x; }
@@ -21,8 +26,10 @@
 // Check that NRVO variables are stored as a pointer with deref if they are
 // stored in the return register.
 
-// CHECK: %result.ptr = alloca i8*, align 8
-// CHECK: call void @llvm.dbg.declare(metadata i8** %result.ptr,
+// CHECK: %[[RESULT:.*]] = alloca i8*, align 8
+// CHECK: call void @llvm.dbg.declare(metadata i8** %[[RESULT]],
 // CHECK-SAME: metadata !DIExpression(DW_OP_deref)
-// NOELIDE: call void @llvm.dbg.declare(metadata %struct.Foo* %foo,
+
+// NOELIDE: %[[FOO:.*]] = alloca %struct.Foo, align 4
+// NOELIDE: call void @llvm.dbg.declare(metadata %struct.Foo* %[[FOO]],
 // NOELIDE-SAME:metadata !DIExpression()


Index: clang/test/CodeGenCXX/debug-info-nrvo.cpp
===
--- clang/test/CodeGenCXX/debug-info-nrvo.cpp
+++ clang/test/CodeGenCXX/debug-info-nrvo.cpp
@@ -1,5 +1,10 @@
-// RUN: %clangxx -target x86_64-unknown-unknown -g %s -emit-llvm -S -o - | FileCheck %s
-// RUN: %clangxx -target x86_64-unknown-unknown -g -fno-elide-constructors %s -emit-llvm -S -o - | FileCheck %s -check-prefix=NOELIDE
+// RUN: %clangxx -target x86_64-unknown-unknown -g \
+// RUN:   %s -emit-llvm -S -o - | FileCheck %s
+
+// RUN: %clangxx -target x86_64-unknown-unknown -g \
+// RUN:   -fno-elide-constructors %s -emit-llvm -S -o - | \
+// RUN:   FileCheck %s -check-prefix=NOELIDE
+
 struct Foo {
   Foo() = default;
   Foo(Foo &&other) { x = other.x; }
@@ -21,8 +26,10 @@
 // Check that NRVO variables are stored as a pointer with deref if they are
 // stored in the return register.
 
-// CHECK: %result.ptr = alloca i8*, align 8
-// CHECK: call void @llvm.dbg.declare(metadata i8** %result.ptr,
+// CHECK: %[[RESULT:.*]] = alloca i8*, align 8
+// CHECK: call void @llvm.dbg.declare(metadata i8** %[[RESULT]],
 // CHECK-SAME: metadata !DIExpression(DW_OP_deref)
-// NOELIDE: call void @llvm.dbg.declare(metadata %struct.Foo* %foo,
+
+// NOELIDE: %[[FOO:.*]] = alloca %struct.Foo, align 4
+// NOELIDE: call void @llvm.dbg.declare(metadata %struct.Foo* %[[FOO]],
 // NOELIDE-SAME:metadata !DIExpression()
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61689: Change -gz and -Wa,--compress-debug-sections to use gABI compression (SHF_COMPRESSED)

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

So, while I think this is an //entirely// reasonable assumption in most cases, 
it's also not one that provides any kind of workaround for the few cases where 
it's not universally true.

- As mentioned in the patch, this effectively changes the default from 
`-gz=zlib-gnu` to `-gz=zlib`. Anyone with an older toolchain that wants the old 
behavior can set `-gz=zlib-gnu`. Seems OK so far.
- However, passing the `-gz` flag //also// implies sending 
`--compress-debug-sections=XXX` to the assembler, which -- if someone is using 
`-no-integrated-as` has an older toolchain -- is not a supported option (at 
least the variant that takes a value).

In other works, for the few users that have an older toolchain, this provides 
literally no workaround. Not setting the flag causes failures with an older 
linker, and setting the flag causes failures with the assembler.

(echristo reverted this in rL360703 , I 
wanted to add some context)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61689



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

[PATCH] D55964: [clang-format][TableGen] Don't add spaces around items in square braces.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht created this revision.
rupprecht added reviewers: djasper, krasimir.
Herald added a subscriber: cfe-commits.

clang-formatting wants to add spaces around items in square braces, e.g. [1, 2] 
-> [ 1, 2 ]. Based on a quick check [1], it seems like most cases are using the 
[1, 2] format, so make that the consistent one.

[1] in llvm `.td` files, the regex `\[[^ ]` (bracket followed by not-a-space) 
shows up ~400 times, but `\[\s[^ ]` (bracket followed by one space and one 
not-a-space) shows up ~40 times => ~90% uses this format.


Repository:
  rC Clang

https://reviews.llvm.org/D55964

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


Index: unittests/Format/FormatTestTableGen.cpp
===
--- unittests/Format/FormatTestTableGen.cpp
+++ unittests/Format/FormatTestTableGen.cpp
@@ -52,5 +52,9 @@
"   \"very long help string\">;\n");
 }
 
+TEST_F(FormatTestTableGen, NoSpacesInSquareBracketLists) {
+  verifyFormat("def flag : Flag<[\"-\", \"--\"], \"foo\">;\n");
+}
+
 } // namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -432,6 +432,10 @@
   } else if (CurrentToken->is(tok::r_square) && Parent &&
  Parent->is(TT_TemplateCloser)) {
 Left->Type = TT_ArraySubscriptLSquare;
+  } else if (Style.Language == FormatStyle::LK_TableGen) {
+// TableGen treats '[' as array subscripts to avoid spaces, e.g.
+// def foo : Flag<["-", "--"], "foo">;
+Left->Type = TT_ArraySubscriptLSquare;
   } else if (Style.Language == FormatStyle::LK_Proto ||
  Style.Language == FormatStyle::LK_TextProto) {
 // Square braces in LK_Proto can either be message field attributes:


Index: unittests/Format/FormatTestTableGen.cpp
===
--- unittests/Format/FormatTestTableGen.cpp
+++ unittests/Format/FormatTestTableGen.cpp
@@ -52,5 +52,9 @@
"   \"very long help string\">;\n");
 }
 
+TEST_F(FormatTestTableGen, NoSpacesInSquareBracketLists) {
+  verifyFormat("def flag : Flag<[\"-\", \"--\"], \"foo\">;\n");
+}
+
 } // namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -432,6 +432,10 @@
   } else if (CurrentToken->is(tok::r_square) && Parent &&
  Parent->is(TT_TemplateCloser)) {
 Left->Type = TT_ArraySubscriptLSquare;
+  } else if (Style.Language == FormatStyle::LK_TableGen) {
+// TableGen treats '[' as array subscripts to avoid spaces, e.g.
+// def foo : Flag<["-", "--"], "foo">;
+Left->Type = TT_ArraySubscriptLSquare;
   } else if (Style.Language == FormatStyle::LK_Proto ||
  Style.Language == FormatStyle::LK_TextProto) {
 // Square braces in LK_Proto can either be message field attributes:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55964: [clang-format][TableGen] Don't add spaces around items in square braces.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Post-holiday ping


Repository:
  rC Clang

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

https://reviews.llvm.org/D55964



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

[PATCH] D55964: [clang-format][TableGen] Don't add spaces around items in square braces.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D55964#1351348 , @djasper wrote:

> This seem to conceptually be a list of things rather than an array subscript, 
> though, right?  Could we alternatively set SpacesInContainerLiterals to false 
> for LK_TableGen?


That seems to work, e.g. if I manually set the option in the test, but I can't 
seem to find a way to set a per-language default. `getLLVMStyle()`, which seems 
to be the default that every style is rooted from, assumes C++ (and does not 
have the language provided). Is there a good way to workaround that?


Repository:
  rC Clang

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

https://reviews.llvm.org/D55964



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

[PATCH] D55964: [clang-format][TableGen] Don't add spaces around items in square braces.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Right, but `getGoogleStyle()` has the inferred language passed in (i.e. 
`getGoogleStyle(FormatStyle::LanguageKind Language)`. Whereas `getLLVMStyle()` 
takes no args and assumes C++.
Would it be worthwhile to refactor getLLVMStyle to take in an optional language 
arg?


Repository:
  rC Clang

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

https://reviews.llvm.org/D55964



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

[PATCH] D55964: [clang-format][TableGen] Don't add spaces around items in square braces.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht updated this revision to Diff 180882.
rupprecht added a comment.

Move TableGen language check to where SpacesInContainerLiterals is checked


Repository:
  rC Clang

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

https://reviews.llvm.org/D55964

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


Index: unittests/Format/FormatTestTableGen.cpp
===
--- unittests/Format/FormatTestTableGen.cpp
+++ unittests/Format/FormatTestTableGen.cpp
@@ -52,5 +52,9 @@
"   \"very long help string\">;\n");
 }
 
+TEST_F(FormatTestTableGen, NoSpacesInSquareBracketLists) {
+  verifyFormat("def flag : Flag<[\"-\", \"--\"], \"foo\">;\n");
+}
+
 } // namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2486,12 +2486,13 @@
 return false;
   const auto SpaceRequiredForArrayInitializerLSquare =
   [](const FormatToken &LSquareTok, const FormatStyle &Style) {
-return Style.SpacesInContainerLiterals ||
-   ((Style.Language == FormatStyle::LK_Proto ||
- Style.Language == FormatStyle::LK_TextProto) &&
-!Style.Cpp11BracedListStyle &&
-LSquareTok.endsSequence(tok::l_square, tok::colon,
-TT_SelectorName));
+return Style.Language != FormatStyle::LK_TableGen &&
+   (Style.SpacesInContainerLiterals ||
+((Style.Language == FormatStyle::LK_Proto ||
+  Style.Language == FormatStyle::LK_TextProto) &&
+ !Style.Cpp11BracedListStyle &&
+ LSquareTok.endsSequence(tok::l_square, tok::colon,
+ TT_SelectorName)));
   };
   if (Left.is(tok::l_square))
 return (Left.is(TT_ArrayInitializerLSquare) && Right.isNot(tok::r_square) 
&&


Index: unittests/Format/FormatTestTableGen.cpp
===
--- unittests/Format/FormatTestTableGen.cpp
+++ unittests/Format/FormatTestTableGen.cpp
@@ -52,5 +52,9 @@
"   \"very long help string\">;\n");
 }
 
+TEST_F(FormatTestTableGen, NoSpacesInSquareBracketLists) {
+  verifyFormat("def flag : Flag<[\"-\", \"--\"], \"foo\">;\n");
+}
+
 } // namespace format
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2486,12 +2486,13 @@
 return false;
   const auto SpaceRequiredForArrayInitializerLSquare =
   [](const FormatToken &LSquareTok, const FormatStyle &Style) {
-return Style.SpacesInContainerLiterals ||
-   ((Style.Language == FormatStyle::LK_Proto ||
- Style.Language == FormatStyle::LK_TextProto) &&
-!Style.Cpp11BracedListStyle &&
-LSquareTok.endsSequence(tok::l_square, tok::colon,
-TT_SelectorName));
+return Style.Language != FormatStyle::LK_TableGen &&
+   (Style.SpacesInContainerLiterals ||
+((Style.Language == FormatStyle::LK_Proto ||
+  Style.Language == FormatStyle::LK_TextProto) &&
+ !Style.Cpp11BracedListStyle &&
+ LSquareTok.endsSequence(tok::l_square, tok::colon,
+ TT_SelectorName)));
   };
   if (Left.is(tok::l_square))
 return (Left.is(TT_ArrayInitializerLSquare) && Right.isNot(tok::r_square) &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48733: Introduce a separate preprocessor macro, _LIBUNWIND_USE_DLADDR, for directly controlling a dependency on dladdr(). This will allow us to use libunwind without adding a libdl dependency

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht created this revision.
rupprecht added a reviewer: saugustine.
Herald added subscribers: cfe-commits, chrib, christof.

Repository:
  rUNW libunwind

https://reviews.llvm.org/D48733

Files:
  src/AddressSpace.hpp


Index: src/AddressSpace.hpp
===
--- src/AddressSpace.hpp
+++ src/AddressSpace.hpp
@@ -18,7 +18,15 @@
 #include 
 #include 
 
-#if !defined(_LIBUNWIND_IS_BAREMETAL) && !defined(_WIN32)
+#ifndef _LIBUNWIND_USE_DLADDR
+  #if !defined(_LIBUNWIND_IS_BAREMETAL) && !defined(_WIN32)
+#define _LIBUNWIND_USE_DLADDR 1
+  #else
+#define _LIBUNWIND_USE_DLADDR 0
+  #endif
+#endif
+
+#if _LIBUNWIND_USE_DLADDR
 #include 
 #endif
 
@@ -576,7 +584,7 @@
 inline bool LocalAddressSpace::findFunctionName(pint_t addr, char *buf,
 size_t bufLen,
 unw_word_t *offset) {
-#if !defined(_LIBUNWIND_IS_BAREMETAL) && !defined(_WIN32)
+#if _LIBUNWIND_USE_DLADDR
   Dl_info dyldInfo;
   if (dladdr((void *)addr, &dyldInfo)) {
 if (dyldInfo.dli_sname != NULL) {


Index: src/AddressSpace.hpp
===
--- src/AddressSpace.hpp
+++ src/AddressSpace.hpp
@@ -18,7 +18,15 @@
 #include 
 #include 
 
-#if !defined(_LIBUNWIND_IS_BAREMETAL) && !defined(_WIN32)
+#ifndef _LIBUNWIND_USE_DLADDR
+  #if !defined(_LIBUNWIND_IS_BAREMETAL) && !defined(_WIN32)
+#define _LIBUNWIND_USE_DLADDR 1
+  #else
+#define _LIBUNWIND_USE_DLADDR 0
+  #endif
+#endif
+
+#if _LIBUNWIND_USE_DLADDR
 #include 
 #endif
 
@@ -576,7 +584,7 @@
 inline bool LocalAddressSpace::findFunctionName(pint_t addr, char *buf,
 size_t bufLen,
 unw_word_t *offset) {
-#if !defined(_LIBUNWIND_IS_BAREMETAL) && !defined(_WIN32)
+#if _LIBUNWIND_USE_DLADDR
   Dl_info dyldInfo;
   if (dladdr((void *)addr, &dyldInfo)) {
 if (dyldInfo.dli_sname != NULL) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48785: Add a blank line to docs/README.txt test commit access

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht created this revision.
Herald added subscribers: cfe-commits, christof.

Repository:
  rUNW libunwind

https://reviews.llvm.org/D48785

Files:
  docs/README.txt


Index: docs/README.txt
===
--- docs/README.txt
+++ docs/README.txt
@@ -11,3 +11,4 @@
 
 After configuring libunwind with these options the make rule 
`docs-libunwind-html`
 should be available.
+


Index: docs/README.txt
===
--- docs/README.txt
+++ docs/README.txt
@@ -11,3 +11,4 @@
 
 After configuring libunwind with these options the make rule `docs-libunwind-html`
 should be available.
+
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48785: Add a blank line to docs/README.txt test commit access

[Jordan Rupprecht via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL336006: Add a blank line to docs/README.txt test commit 
access (authored by rupprecht, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D48785

Files:
  libunwind/trunk/docs/README.txt


Index: libunwind/trunk/docs/README.txt
===
--- libunwind/trunk/docs/README.txt
+++ libunwind/trunk/docs/README.txt
@@ -11,3 +11,4 @@
 
 After configuring libunwind with these options the make rule 
`docs-libunwind-html`
 should be available.
+


Index: libunwind/trunk/docs/README.txt
===
--- libunwind/trunk/docs/README.txt
+++ libunwind/trunk/docs/README.txt
@@ -11,3 +11,4 @@
 
 After configuring libunwind with these options the make rule `docs-libunwind-html`
 should be available.
+
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48787: Un-revert "Support for multiarch runtimes layout"This reverts https://reviews.llvm.org/rL336005, which was an accidental commit.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht created this revision.
Herald added subscribers: cfe-commits, christof, mgorny.

Repository:
  rUNW libunwind

https://reviews.llvm.org/D48787

Files:
  CMakeLists.txt


Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -170,7 +170,14 @@
 set(LIBUNWIND_COMPILER${CMAKE_CXX_COMPILER})
 set(LIBUNWIND_SOURCE_DIR  ${CMAKE_CURRENT_SOURCE_DIR})
 set(LIBUNWIND_BINARY_DIR  ${CMAKE_CURRENT_BINARY_DIR})
-if (LLVM_LIBRARY_OUTPUT_INTDIR)
+
+string(REGEX MATCH "[0-9]+\\.[0-9]+(\\.[0-9]+)?" CLANG_VERSION
+   ${PACKAGE_VERSION})
+
+if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
+  set(DEFAULT_INSTALL_PREFIX 
lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION}/${LLVM_DEFAULT_TARGET_TRIPLE}/)
+  set(LIBUNWIND_LIBRARY_DIR 
${LLVM_LIBRARY_OUTPUT_INTDIR}/clang/${CLANG_VERSION}/${LLVM_DEFAULT_TARGET_TRIPLE}/lib${LLVM_RUNTIMES_LIBDIR_SUFFIX})
+elseif(LLVM_LIBRARY_OUTPUT_INTDIR)
   set(LIBUNWIND_LIBRARY_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR})
 else()
   set(LIBUNWIND_LIBRARY_DIR ${CMAKE_BINARY_DIR}/lib${LIBUNWIND_LIBDIR_SUFFIX})
@@ -180,13 +187,9 @@
 set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${LIBUNWIND_LIBRARY_DIR})
 set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${LIBUNWIND_LIBRARY_DIR})
 
-set(LIBUNWIND_INSTALL_PREFIX "" CACHE STRING
+set(LIBUNWIND_INSTALL_PREFIX ${DEFAULT_INSTALL_PREFIX} CACHE STRING
 "Define libunwind destination prefix.")
 
-if (NOT LIBUNWIND_INSTALL_PREFIX MATCHES "^$|.*/")
-  message(FATAL_ERROR "LIBUNWIND_INSTALL_PREFIX has to end with \"/\".")
-endif()
-
 set(LIBUNWIND_C_FLAGS "")
 set(LIBUNWIND_CXX_FLAGS "")
 set(LIBUNWIND_COMPILE_FLAGS "")


Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -170,7 +170,14 @@
 set(LIBUNWIND_COMPILER${CMAKE_CXX_COMPILER})
 set(LIBUNWIND_SOURCE_DIR  ${CMAKE_CURRENT_SOURCE_DIR})
 set(LIBUNWIND_BINARY_DIR  ${CMAKE_CURRENT_BINARY_DIR})
-if (LLVM_LIBRARY_OUTPUT_INTDIR)
+
+string(REGEX MATCH "[0-9]+\\.[0-9]+(\\.[0-9]+)?" CLANG_VERSION
+   ${PACKAGE_VERSION})
+
+if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
+  set(DEFAULT_INSTALL_PREFIX lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION}/${LLVM_DEFAULT_TARGET_TRIPLE}/)
+  set(LIBUNWIND_LIBRARY_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR}/clang/${CLANG_VERSION}/${LLVM_DEFAULT_TARGET_TRIPLE}/lib${LLVM_RUNTIMES_LIBDIR_SUFFIX})
+elseif(LLVM_LIBRARY_OUTPUT_INTDIR)
   set(LIBUNWIND_LIBRARY_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR})
 else()
   set(LIBUNWIND_LIBRARY_DIR ${CMAKE_BINARY_DIR}/lib${LIBUNWIND_LIBDIR_SUFFIX})
@@ -180,13 +187,9 @@
 set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${LIBUNWIND_LIBRARY_DIR})
 set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${LIBUNWIND_LIBRARY_DIR})
 
-set(LIBUNWIND_INSTALL_PREFIX "" CACHE STRING
+set(LIBUNWIND_INSTALL_PREFIX ${DEFAULT_INSTALL_PREFIX} CACHE STRING
 "Define libunwind destination prefix.")
 
-if (NOT LIBUNWIND_INSTALL_PREFIX MATCHES "^$|.*/")
-  message(FATAL_ERROR "LIBUNWIND_INSTALL_PREFIX has to end with \"/\".")
-endif()
-
 set(LIBUNWIND_C_FLAGS "")
 set(LIBUNWIND_CXX_FLAGS "")
 set(LIBUNWIND_COMPILE_FLAGS "")
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48733: Introduce a separate preprocessor macro, _LIBUNWIND_USE_DLADDR, for directly controlling a dependency on dladdr(). This will allow us to use libunwind without adding a libdl dependency

[Jordan Rupprecht via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL336014: Introduce a separate preprocessor macro, 
_LIBUNWIND_USE_DLADDR, for directly… (authored by rupprecht, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D48733

Files:
  libunwind/trunk/src/AddressSpace.hpp


Index: libunwind/trunk/src/AddressSpace.hpp
===
--- libunwind/trunk/src/AddressSpace.hpp
+++ libunwind/trunk/src/AddressSpace.hpp
@@ -18,7 +18,15 @@
 #include 
 #include 
 
-#if !defined(_LIBUNWIND_IS_BAREMETAL) && !defined(_WIN32)
+#ifndef _LIBUNWIND_USE_DLADDR
+  #if !defined(_LIBUNWIND_IS_BAREMETAL) && !defined(_WIN32)
+#define _LIBUNWIND_USE_DLADDR 1
+  #else
+#define _LIBUNWIND_USE_DLADDR 0
+  #endif
+#endif
+
+#if _LIBUNWIND_USE_DLADDR
 #include 
 #endif
 
@@ -576,7 +584,7 @@
 inline bool LocalAddressSpace::findFunctionName(pint_t addr, char *buf,
 size_t bufLen,
 unw_word_t *offset) {
-#if !defined(_LIBUNWIND_IS_BAREMETAL) && !defined(_WIN32)
+#if _LIBUNWIND_USE_DLADDR
   Dl_info dyldInfo;
   if (dladdr((void *)addr, &dyldInfo)) {
 if (dyldInfo.dli_sname != NULL) {


Index: libunwind/trunk/src/AddressSpace.hpp
===
--- libunwind/trunk/src/AddressSpace.hpp
+++ libunwind/trunk/src/AddressSpace.hpp
@@ -18,7 +18,15 @@
 #include 
 #include 
 
-#if !defined(_LIBUNWIND_IS_BAREMETAL) && !defined(_WIN32)
+#ifndef _LIBUNWIND_USE_DLADDR
+  #if !defined(_LIBUNWIND_IS_BAREMETAL) && !defined(_WIN32)
+#define _LIBUNWIND_USE_DLADDR 1
+  #else
+#define _LIBUNWIND_USE_DLADDR 0
+  #endif
+#endif
+
+#if _LIBUNWIND_USE_DLADDR
 #include 
 #endif
 
@@ -576,7 +584,7 @@
 inline bool LocalAddressSpace::findFunctionName(pint_t addr, char *buf,
 size_t bufLen,
 unw_word_t *offset) {
-#if !defined(_LIBUNWIND_IS_BAREMETAL) && !defined(_WIN32)
+#if _LIBUNWIND_USE_DLADDR
   Dl_info dyldInfo;
   if (dladdr((void *)addr, &dyldInfo)) {
 if (dyldInfo.dli_sname != NULL) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49330: [compiler-rt] Include -lm when using compiler-rt, due to dependencies in some __div methods.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht created this revision.
rupprecht added a reviewer: saugustine.
Herald added subscribers: cfe-commits, aheejin, sbc100, dberris, srhines.

Some methods in compiler-rt builtins have dependencies on libm. Therefore 
building with -rtlib=compiler-rt is completely broken if those symbols are used 
and users don't include -lm themselves.

Fixes PR32279 and PR28652.


Repository:
  rC Clang

https://reviews.llvm.org/D49330

Files:
  lib/Driver/ToolChains/CommonArgs.cpp
  test/Driver/linux-ld.c
  test/Driver/wasm-toolchain.c
  test/Driver/wasm-toolchain.cpp


Index: test/Driver/wasm-toolchain.cpp
===
--- test/Driver/wasm-toolchain.cpp
+++ test/Driver/wasm-toolchain.cpp
@@ -14,10 +14,10 @@
 
 // RUN: %clangxx -### -no-canonical-prefixes -target wasm32-unknown-unknown 
--sysroot=/foo --stdlib=c++ %s 2>&1 | FileCheck -check-prefix=LINK %s
 // LINK: clang{{.*}}" "-cc1" {{.*}} "-o" "[[temp:[^"]*]]"
-// LINK: lld{{.*}}" "-flavor" "wasm" "-L/foo/lib" "crt1.o" "[[temp]]" "-lc++" 
"-lc++abi" "-lc" "{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-o" "a.out"
+// LINK: lld{{.*}}" "-flavor" "wasm" "-L/foo/lib" "crt1.o" "[[temp]]" "-lc++" 
"-lc++abi" "-lc" "{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-lm" "-o" "a.out"
 
 // A basic C++ link command-line with optimization.
 
 // RUN: %clangxx -### -O2 -no-canonical-prefixes -target 
wasm32-unknown-unknown --sysroot=/foo %s --stdlib=c++ 2>&1 | FileCheck 
-check-prefix=LINK_OPT %s
 // LINK_OPT: clang{{.*}}" "-cc1" {{.*}} "-o" "[[temp:[^"]*]]"
-// LINK_OPT: lld{{.*}}" "-flavor" "wasm" "-L/foo/lib" "crt1.o" "[[temp]]" 
"-lc++" "-lc++abi" "-lc" "{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-o" "a.out"
+// LINK_OPT: lld{{.*}}" "-flavor" "wasm" "-L/foo/lib" "crt1.o" "[[temp]]" 
"-lc++" "-lc++abi" "-lc" "{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-lm" "-o" 
"a.out"
Index: test/Driver/wasm-toolchain.c
===
--- test/Driver/wasm-toolchain.c
+++ test/Driver/wasm-toolchain.c
@@ -14,10 +14,10 @@
 
 // RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown 
--sysroot=/foo %s 2>&1 | FileCheck -check-prefix=LINK %s
 // LINK: clang{{.*}}" "-cc1" {{.*}} "-o" "[[temp:[^"]*]]"
-// LINK: lld{{.*}}" "-flavor" "wasm" "-L/foo/lib" "crt1.o" "[[temp]]" "-lc" 
"{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-o" "a.out"
+// LINK: lld{{.*}}" "-flavor" "wasm" "-L/foo/lib" "crt1.o" "[[temp]]" "-lc" 
"{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-lm" "-o" "a.out"
 
 // A basic C link command-line with optimization.
 
 // RUN: %clang -### -O2 -no-canonical-prefixes -target wasm32-unknown-unknown 
--sysroot=/foo %s 2>&1 | FileCheck -check-prefix=LINK_OPT %s
 // LINK_OPT: clang{{.*}}" "-cc1" {{.*}} "-o" "[[temp:[^"]*]]"
-// LINK_OPT: lld{{.*}}" "-flavor" "wasm" "-L/foo/lib" "crt1.o" "[[temp]]" 
"-lc" "{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-o" "a.out"
+// LINK_OPT: lld{{.*}}" "-flavor" "wasm" "-L/foo/lib" "crt1.o" "[[temp]]" 
"-lc" "{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-lm" "-o" "a.out"
Index: test/Driver/linux-ld.c
===
--- test/Driver/linux-ld.c
+++ test/Driver/linux-ld.c
@@ -67,8 +67,10 @@
 // CHECK-LD-RT: "-L[[SYSROOT]]/lib"
 // CHECK-LD-RT: "-L[[SYSROOT]]/usr/lib"
 // CHECK-LD-RT: libclang_rt.builtins-x86_64.a"
+// CHECK-LD-RT: "-lm"
 // CHECK-LD-RT: "-lc"
 // CHECK-LD-RT: libclang_rt.builtins-x86_64.a"
+// CHECK-LD-RT: "-lm"
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: --target=i686-unknown-linux \
@@ -88,8 +90,10 @@
 // CHECK-LD-RT-I686: "-L[[SYSROOT]]/lib"
 // CHECK-LD-RT-I686: "-L[[SYSROOT]]/usr/lib"
 // CHECK-LD-RT-I686: libclang_rt.builtins-i386.a"
+// CHECK-LD-RT-I686: "-lm"
 // CHECK-LD-RT-I686: "-lc"
 // CHECK-LD-RT-I686: libclang_rt.builtins-i386.a"
+// CHECK-LD-RT-I686: "-lm"
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: --target=arm-linux-androideabi \
@@ -103,8 +107,10 @@
 // CHECK-LD-RT-ANDROID: "-m" "armelf_linux_eabi"
 // CHECK-LD-RT-ANDROID: "-dynamic-linker"
 // CHECK-LD-RT-ANDROID: libclang_rt.builtins-arm-android.a"
+// CHECK-LD-RT-ANDROID: "-lm"
 // CHECK-LD-RT-ANDROID: "-lc"
 // CHECK-LD-RT-ANDROID: libclang_rt.builtins-arm-android.a"
+// CHECK-LD-RT-ANDROID: "-lm"
 //
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: --target=x86_64-unknown-linux -rtlib=platform \
Index: lib/Driver/ToolChains/CommonArgs.cpp
===
--- lib/Driver/ToolChains/CommonArgs.cpp
+++ lib/Driver/ToolChains/CommonArgs.cpp
@@ -1162,6 +1162,8 @@
   switch (RLT) {
   case ToolChain::RLT_CompilerRT:
 CmdArgs.push_back(TC.getCompilerRTArgString(Args, "builtins"));
+// Some methods such as __divdc3, __divsc3, and __divxc3 rely on libm
+CmdArgs.push_back("-lm");
 break;
   case ToolChain::RLT_Libgcc:
 // Make sure libgcc is not used under MSV

[PATCH] D49330: [compiler-rt] Include -lm when using compiler-rt, due to dependencies in some __div methods.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Thanks Eli, I'll see how hard it is to remove those calls to logb/logbf (those 
seem to be the only ones causing link errors) and revert this change if that's 
possible.


Repository:
  rC Clang

https://reviews.llvm.org/D49330



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

[PATCH] D68669: [llvm-objdump][WIP] Make llvm-objdump -h compatible with GNU objdump.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht planned changes to this revision.
rupprecht added a comment.

Note: herald added reviewers, but this patch is just to provide context. I'll 
send the real patches for review in the coming days.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68669



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

[PATCH] D68669: [llvm-objdump][WIP] Make llvm-objdump -h compatible with GNU objdump.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht created this revision.
Herald added subscribers: llvm-commits, cfe-commits, seiya, arphaman, 
jakehehrlich, aheejin, arichardson, sbc100, emaste.
Herald added a reviewer: espindola.
Herald added a reviewer: alexshap.
Herald added a reviewer: jhenderson.
Herald added projects: clang, LLVM.
rupprecht planned changes to this revision.
rupprecht added a comment.

Note: herald added reviewers, but this patch is just to provide context. I'll 
send the real patches for review in the coming days.


Note: this patch is large and not intended for submission as-is. Instead, this 
patch presents a poor implementation that makes llvm-objdump GNU compatibile 
for this option (with all existing tests passing, but few added tests, hacky 
code, etc.), and will serve as context for smaller changes to be submitted 
separately with more careful review.

llvm-objdump -h was implemented to be similar to readelf -S (see rL141579 
). However, it is not completely compatible 
with that, and anyone that does want headers displayed that way can use 
llvm-readelf -S now that it exists. Make llvm-objdump compatible with GNU 
objdump instead.

A brief overview of changes:

- Add file offset (with implementations for all filetypes supported by 
llvm-readobj).
- Add 2**n section alignment column (with implementations for all filetypes 
supported by llvm-readobj).
- Section numbers are not the actual section numbers, but something different, 
corresponding to libbfd section numbers. llvm-readelf -s prints the actual 
section numbers if those are desired.
- Filter out certain sections like symtabs/strtabs/relocs. The actual logic is 
a lot more convoluted (and probably isn't fully compatibile, but is pretty 
close).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D68669

Files:
  clang/test/Modules/pch_container.m
  lld/test/ELF/bss-start-common.s
  lld/test/ELF/edata-etext.s
  lld/test/ELF/edata-no-bss.s
  lld/test/ELF/emit-relocs-gc.s
  lld/test/ELF/gc-sections-metadata.s
  lld/test/ELF/init_fini_priority.s
  lld/test/ELF/invalid-fde-rel.s
  lld/test/ELF/linkerscript/addr.test
  lld/test/ELF/linkerscript/align-empty.test
  lld/test/ELF/linkerscript/align1.test
  lld/test/ELF/linkerscript/align2.test
  lld/test/ELF/linkerscript/align3.test
  lld/test/ELF/linkerscript/at2.test
  lld/test/ELF/linkerscript/constructor.test
  lld/test/ELF/linkerscript/define.test
  lld/test/ELF/linkerscript/double-bss.test
  lld/test/ELF/linkerscript/eh-frame-emit-relocs.s
  lld/test/ELF/linkerscript/emit-reloc-section-names.s
  lld/test/ELF/linkerscript/expr-sections.test
  lld/test/ELF/linkerscript/input-sec-dup.s
  lld/test/ELF/linkerscript/insert-after.test
  lld/test/ELF/linkerscript/insert-before.test
  lld/test/ELF/linkerscript/memory-include.test
  lld/test/ELF/linkerscript/memory.s
  lld/test/ELF/linkerscript/memory3.s
  lld/test/ELF/linkerscript/memory4.test
  lld/test/ELF/linkerscript/memory5.test
  lld/test/ELF/linkerscript/multi-sections-constraint.s
  lld/test/ELF/linkerscript/non-absolute2.test
  lld/test/ELF/linkerscript/numbers.s
  lld/test/ELF/linkerscript/orphan.s
  lld/test/ELF/linkerscript/orphans.s
  lld/test/ELF/linkerscript/out-of-order-section-in-region.test
  lld/test/ELF/linkerscript/out-of-order.s
  lld/test/ELF/linkerscript/output-section-include.test
  lld/test/ELF/linkerscript/region-alias.s
  lld/test/ELF/linkerscript/repsection-va.s
  lld/test/ELF/linkerscript/section-include.test
  lld/test/ELF/linkerscript/sections-constraint.s
  lld/test/ELF/linkerscript/sections-gc2.s
  lld/test/ELF/linkerscript/sections-keep.s
  lld/test/ELF/linkerscript/sections-sort.s
  lld/test/ELF/linkerscript/sections.s
  lld/test/ELF/linkerscript/sizeof.s
  lld/test/ELF/linkerscript/symbol-only.test
  lld/test/ELF/linkerscript/va.s
  lld/test/ELF/linkerscript/wildcards.s
  lld/test/ELF/linkerscript/wildcards2.s
  lld/test/ELF/relocatable-sections.s
  lld/test/ELF/relocatable.s
  lld/test/ELF/relro-omagic.s
  lld/test/ELF/section-name.s
  lld/test/ELF/sectionstart-noallochdr.s
  lld/test/ELF/sectionstart.s
  lld/test/ELF/strip-all.s
  lld/test/ELF/synthetic-got.s
  llvm/test/MC/COFF/assoc-private.s
  llvm/test/Object/objdump-no-sectionheaders.test
  llvm/test/Object/objdump-sectionheaders.test
  llvm/test/ObjectYAML/CodeView/sections.yaml
  llvm/test/tools/llvm-objcopy/ELF/symtab-error-on-remove-strtab.test
  llvm/test/tools/llvm-objdump/X86/adjust-vma.test
  llvm/test/tools/llvm-objdump/X86/macho-section-headers.test
  llvm/test/tools/llvm-objdump/X86/phdrs-lma.test
  llvm/test/tools/llvm-objdump/X86/phdrs-lma2.test
  llvm/test/tools/llvm-objdump/X86/section-index.s
  llvm/test/tools/llvm-objdump/section-filter.test
  llvm/test/tools/llvm-objdump/wasm.txt
  llvm/test/tools/llvm-objdump/xcoff-section-headers.test
  llvm/tools/llvm-objdump/llvm-objdump.cpp
  llvm/tools/llvm-objdump/llvm-objdump.h

Index: llvm/tools/llvm-objdump/llvm-objdump.h
===

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Just wanna say huge +1 for this patch. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67122



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

[PATCH] D66490: [NewPM] Enable the New Pass Manager by Default in Clang

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

We already know that we don't want this enabled for tsan builds due to 
https://bugs.llvm.org/show_bug.cgi?id=42877, but I don't even know if anyone 
else will hit it (it's only when building one particular library).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66490



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

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

> Still think this looks good. Have you tried running this on the llvm test 
> suite, or some other interesting corpus? Would be curious to see any pre/post 
> patch numbers.

I finally had time this morning to patch this in and give it a shot. (Sorry for 
the delay... it's been a real busy week :( )

First, starting off with the good news: I reverted all the fixes I made, and 
now all the tests fail when running w/ ubsan. Yay!

The error we see in each case is `UndefinedBehaviorSanitizer: 
nullptr-with-nonzero-offset` with the logs containing `runtime error: applying 
non-zero offset  to null pointer`. Which catches the two places where 
we were adding some non-zero offset to nullptr, but doesn't seem to catch the 
nullptr-after-nonzero-offset case in 
https://github.com/google/filament/pull/1566 -- instead, it fails later, when 
the pointer with a value of nullptr is incremented. (Or... maybe this is 
actually a separate bug. Hmm. Needs some more testing...)

At any rate, I have some more tests to run to get some idea of what % of code 
this would flag as being bad.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67122



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

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

> But TLDR, either the fix in https://github.com/google/filament/pull/1566
>  is incorrect and the actually-bad code is elsewhere,
>  or you have some other unsanitized UB elsewhere. Could be both :S

My money is on "both" :p

I tested a random sample of a couple thousand tests internally and ~1% of them 
failed, but when I looked at them, they were all coming from two separate 
widely used libraries. I fixed those, and I'll do a broader test now to see how 
bad the long tail of issues are.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67122



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

[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

There's definitely a lot of new findings this creates, but it's hard to say 
exactly how many root causes there are due to the way test failures are (not) 
grouped well in the way I'm testing. So far they all seem like true positives, 
so this would be good to submit. However a few are positive yet benign, like 
this interesting one (simplified):

  void ParseString(char *s) {
char *next = s;
for (char *end = s; end; next = end + 1) { // ubsan error computing (nil + 
1), although it doesn't matter because the loop terminates when end == nil and 
next is not read after the loop
  // ...
  end = strchr(next, 'x'); // returns null if not found
  // ...
}
  }

If I had to guesstimate, I'd say 20-100 bugs in a couple billion lines of code, 
so a lot, but shouldn't be too disruptive to anyone that has these checks 
enabled globally.

I haven't noticed any timeouts -- which is not to say this isn't a slowdown, 
but at least it's not egregious.

BTW, here's a minimal + complete repro of the original issue:

  $ cat ub.cc
  #include 
  #include 
  
  static void Test(const char *x, int offset) {
printf("%p + %d => %s\n", x, offset, x + offset ? "true" : "false");
  }
  
  int main(int argc, char **argv) {
if (argc != 3) return 1;
  
const char *x = reinterpret_cast(atoi(argv[1]));
int offset = atoi(argv[2]);
  
Test(x, offset);
  
return 0;
  }
  $ previous-clang++ -O3 ub.cc && ./a.out 0 1
  (nil) + 1 => true
  $ next-clang++ -O3 ub.cc && ./a.out 0 1
  (nil) + 1 => false
  $ patch-D67122-clang++ -O3 -fsanitize=undefined ub.cc && ./a.out 0 1
  ubsan: ub.cc:5:42: runtime error: applying non-zero offset 1 to null pointer  

  
  (nil) + 1 => false


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67122



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

[PATCH] D71554: [llvm-ranlib] Handle -D and -U command line flag

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D71554#1789360 , @arichardson wrote:

> Also handle -h/-v as short options. Does the adjusted test look okay?


Sorry, didn't have time to take a second look before the holiday break -- yep, 
looks good, I didn't anticipate so many buildbot failures. Would be nice if the 
precommit bot tested Windows too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71554



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

[PATCH] D86290: Move all fields of '-cc1' option related classes into def file databases

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Thanks for the revert.

In addition to the one eabi test, we saw widespread msan 
use-of-uninitialized-value errors from here: 
https://github.com/llvm/llvm-project/blob/master/llvm/lib/Target/ARM/ARMTargetMachine.cpp#L229.
 I think it would explain the eabi test failure.

See also: 
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap-msan/builds/21663/steps/check-clang%20msan/logs/stdio


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86290

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

[PATCH] D88640: [Format] Don't treat compound extension headers (foo.proto.h) as foo.cc main file.

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

It looks like this fix caused a different regression in not accepting 
`name..h` as the main header for `name..cc`, e.g.:

  $ cat /tmp/foo.bar.cc
  #include "a.h"
  #include "z.h"
  #include "foo.bar.h"
  
  $ clang-format /tmp/foo.bar.cc  # Before
  #include "foo.bar.h"
  
  #include "a.h"
  #include "z.h"
  
  $ clang-format /tmp/foo.bar.cc  # After
  #include "a.h"
  #include "foo.bar.h"
  #include "z.h"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88640

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

I'm seeing failures which I think are due to this patch -- I don't have a nice 
godbolt repro yet, but it's something like:

  foo.h: 
  class Foo {
   public:
static void DoStuff();  // Grabs mu_
  
  private:
static std::vector blah_ GUARDED_BY(mu_);
static Mutex mu_;
  };
  
  bar.h:
  class Bar {
static void DoThings(); // calls Foo::DoStuff()
  };

Because `DoStuff()` grabs `mu_`, it needs the lock, and this patch gives an 
error like `requires negative capability 'mu_'`. That's fine, and in fact 
better analysis is welcome.

However, I'm also seeing the same error for `DoThings()`, which doesn't make 
sense. When I add it, I then get errors that `mu_` is private, and therefore 
needs to be made public or friended for the thread analysis -- which is not at 
all a good change (the mutex should be encapsulated in the class). Even though 
it's "global" in the linkage sense, it's not visible outside of the class.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D84604#2363445 , @aaronpuchert 
wrote:

> Pushed a fix in rGbbed8cfe80cd27d3a47d877c7608d9be4e487d97 
> . For 
> now we just consider all static members as inaccessible, so we'll treat them 
> as we did before this change.

I can confirm this fixes the breakage -- thanks!

> I have proposed making the check stronger for non-static members in D87194 
> , perhaps it makes sense to extend this to 
> static members as well so that it fires on `DoStuff()` again.

I applied D87194  locally and rebuilt the 
original source, and not only am I seeing the original issue (also firing on 
`DoThings()` when it should only be on `DoStuff()`), I'm also seeing: `error: 
acquiring mutex 'lock' requires negative capability '!lock' 
[-Werror,-Wthread-safety-negative]`, where `lock` is a local variable, defined 
as `MutexLock lock(mutex_)`.

I'll work on getting a better repro for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D84604: Thread safety analysis: Consider global variables in scope

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D84604#2364568 , @aaronpuchert 
wrote:

> In D84604#2363768 , @rupprecht wrote:
>
>> I applied D87194  locally and rebuilt the 
>> original source, and not only am I seeing the original issue (also firing on 
>> `DoThings()` when it should only be on `DoStuff()`), I'm also seeing: 
>> `error: acquiring mutex 'lock' requires negative capability '!lock' 
>> [-Werror,-Wthread-safety-negative]`, where `lock` is a local variable, 
>> defined as `MutexLock lock(mutex_)`.
>
> Oh yes, I need to rebase this, sorry if I wasted your time. This is still on 
> top of the bug that @lebedev.ri pointed out in D84604#2262745 
>  and on top of the bug that you 
> pointed out.

No worries, it was only a minute to patch, and then a few more minutes to take 
a snack break while it rebuilt :)

>> I'll work on getting a better repro for this.
>
> Maybe wait a bit with that, I'll add you as reviewer when I've done the 
> rebase and then you can try it again. I hope to have covered both locals and 
> static members now.
>
> Another issue is linkage, but I'll have to read up on that a bit.

Yep, I'm not qualified to actually review the code, but I'd be happy to test 
any patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84604

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

[PATCH] D87194: Thread safety analysis: Use access specifiers to decide about scope

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D87194#2369485 , @aaronpuchert 
wrote:

> @rupprecht, maybe you can try it again?

Some more interesting errors this time :)

The ones I originally saw look correct now (i.e. it's flagging the things that 
are valid, but not the things out of visibility). I tried building the rest of 
this package, and I guess scoping isn't considered in this case though?

  class Foo {
   public:
static void Bar();
   private:
struct Params {
  Mutex mu_;
}
static Params* GetParams();
  };
  
  // In the .cc file:
  void Foo::Bar() {
Params& params = *GetParams();
MutexLock lock(params.mu_);  // error: acquiring mutex 'params.mu_' 
requires negative capability '!params.mu_'
  }
  
  /* static */ Params* Foo::GetParams() {
static Params params;
return ¶ms;
  }

On one hand, it's totally valid. On the other hand, annotating the method like 
`static void Bar() REQUIRES(!params.mu_);` isn't possible because `params` is a 
local variable.

(I'm new to threading analysis, so maybe I'm just using the wrong annotations)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87194

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

[PATCH] D101191: [InstCombine] Fully disable select to and/or i1 folding

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

FYI, I'm seeing what I think is a miscompile that bisects to this patch. 
Greatly simplified, the problematic snippet is this:

  struct Stats {
int a;
int b;
int a_or_b;
  };
  
  bool x = ...
  bool y = ...
  Stats stats;
  stats.a = x ? 1 : 0;
  stats.b = y ? 1 : 0;
  stats.a_or_b = (x || y) ? 1 : 0;

What we see is that when x is false and y is true, a is 0 (expected), b is 1 
(expected), but a_or_b is 0 (unexpected).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101191

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

[PATCH] D101191: [InstCombine] Fully disable select to and/or i1 folding

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

The issue I'm seeing seems more directly caused by SLP vectorization, as it 
goes away with `-fno-slp-vectorize`. This patch merely unblocks that bad 
optimization AFAICT.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101191

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

[PATCH] D101191: [InstCombine] Fully disable select to and/or i1 folding

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D101191#2782963 , @rupprecht wrote:

> The issue I'm seeing seems more directly caused by SLP vectorization, as it 
> goes away with `-fno-slp-vectorize`. This patch merely unblocks that bad 
> optimization AFAICT.

Filed as http://llvm.org/PR50500


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101191

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

[PATCH] D97009: [CUDA] fix builtin constraints for PTX 7.2

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

This fixes a build error we're seeing, so I'd like to land this in a bit if 
that's OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97009

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

[PATCH] D97009: [CUDA] fix builtin constraints for PTX 7.2

[Jordan Rupprecht via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1a368ae3b78d: [CUDA] fix builtin constraints for PTX 7.2 
(authored by tra, committed by rupprecht).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97009

Files:
  clang/include/clang/Basic/BuiltinsNVPTX.def
  clang/test/CodeGen/builtins-nvptx-sm_70.cu


Index: clang/test/CodeGen/builtins-nvptx-sm_70.cu
===
--- clang/test/CodeGen/builtins-nvptx-sm_70.cu
+++ clang/test/CodeGen/builtins-nvptx-sm_70.cu
@@ -6,6 +6,11 @@
 // RUN:-fcuda-is-device -target-feature +ptx61 -DPTX61 \
 // RUN:-S -emit-llvm -o - -x cuda %s \
 // RUN:   | FileCheck -check-prefixes=CHECK_M16,CHECK_M32_M8 %s
+// Make sure builtins still work with the latest combination of GPU & PTX.
+// RUN: %clang_cc1 -triple nvptx64-unknown-unknown -target-cpu sm_86 \
+// RUN:-fcuda-is-device -target-feature +ptx72 -DPTX61 \
+// RUN:-S -emit-llvm -o - -x cuda %s \
+// RUN:   | FileCheck -check-prefixes=CHECK_M16,CHECK_M32_M8 %s
 // RUN: %clang_cc1 -triple nvptx-unknown-unknown -target-cpu sm_60 \
 // RUN:   -DPTX61 -fcuda-is-device -S -o /dev/null -x cuda -verify=pre-sm_70 %s
 // RUN: %clang_cc1 -triple nvptx-unknown-unknown \
Index: clang/include/clang/Basic/BuiltinsNVPTX.def
===
--- clang/include/clang/Basic/BuiltinsNVPTX.def
+++ clang/include/clang/Basic/BuiltinsNVPTX.def
@@ -38,7 +38,9 @@
 #pragma push_macro("PTX65")
 #pragma push_macro("PTX70")
 #pragma push_macro("PTX71")
-#define PTX71 "ptx71"
+#pragma push_macro("PTX72")
+#define PTX72 "ptx72"
+#define PTX71 "ptx71|" PTX72
 #define PTX70 "ptx70|" PTX71
 #define PTX65 "ptx65|" PTX70
 #define PTX64 "ptx64|" PTX65
@@ -740,3 +742,4 @@
 #pragma pop_macro("PTX65")
 #pragma pop_macro("PTX70")
 #pragma pop_macro("PTX71")
+#pragma pop_macro("PTX72")


Index: clang/test/CodeGen/builtins-nvptx-sm_70.cu
===
--- clang/test/CodeGen/builtins-nvptx-sm_70.cu
+++ clang/test/CodeGen/builtins-nvptx-sm_70.cu
@@ -6,6 +6,11 @@
 // RUN:-fcuda-is-device -target-feature +ptx61 -DPTX61 \
 // RUN:-S -emit-llvm -o - -x cuda %s \
 // RUN:   | FileCheck -check-prefixes=CHECK_M16,CHECK_M32_M8 %s
+// Make sure builtins still work with the latest combination of GPU & PTX.
+// RUN: %clang_cc1 -triple nvptx64-unknown-unknown -target-cpu sm_86 \
+// RUN:-fcuda-is-device -target-feature +ptx72 -DPTX61 \
+// RUN:-S -emit-llvm -o - -x cuda %s \
+// RUN:   | FileCheck -check-prefixes=CHECK_M16,CHECK_M32_M8 %s
 // RUN: %clang_cc1 -triple nvptx-unknown-unknown -target-cpu sm_60 \
 // RUN:   -DPTX61 -fcuda-is-device -S -o /dev/null -x cuda -verify=pre-sm_70 %s
 // RUN: %clang_cc1 -triple nvptx-unknown-unknown \
Index: clang/include/clang/Basic/BuiltinsNVPTX.def
===
--- clang/include/clang/Basic/BuiltinsNVPTX.def
+++ clang/include/clang/Basic/BuiltinsNVPTX.def
@@ -38,7 +38,9 @@
 #pragma push_macro("PTX65")
 #pragma push_macro("PTX70")
 #pragma push_macro("PTX71")
-#define PTX71 "ptx71"
+#pragma push_macro("PTX72")
+#define PTX72 "ptx72"
+#define PTX71 "ptx71|" PTX72
 #define PTX70 "ptx70|" PTX71
 #define PTX65 "ptx65|" PTX70
 #define PTX64 "ptx64|" PTX65
@@ -740,3 +742,4 @@
 #pragma pop_macro("PTX65")
 #pragma pop_macro("PTX70")
 #pragma pop_macro("PTX71")
+#pragma pop_macro("PTX72")
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101191: [InstCombine] Fully disable select to and/or i1 folding

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D101191#2788596 , @spatel wrote:

> In D101191#2783570 , @rupprecht 
> wrote:
>
>> In D101191#2782963 , @rupprecht 
>> wrote:
>>
>>> The issue I'm seeing seems more directly caused by SLP vectorization, as it 
>>> goes away with `-fno-slp-vectorize`. This patch merely unblocks that bad 
>>> optimization AFAICT.
>>
>> Filed as http://llvm.org/PR50500
>
> We needed SLP to get to the problem state in that example, but the bug was in 
> instcombine/instsimplify. 
> Should be fixed with:
> 7bb8bfa0622b 
> 

Thanks Sanjay! This fixes the unreduced miscompile we're seeing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101191

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

[PATCH] D104261: Thread safety analysis: Always warn when dropping locks on back edges

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D104261#2844636 , @aaronpuchert 
wrote:

> In D104261#2841356 , @delesley 
> wrote:
>
>> since it's restricted to relockable managed locks, I'm not too worried...
>
> Not quite, it affects scoped locks with explicit unlock, which was supported 
> before D49885 .
>
> @rupprecht, can you still test patches on Google's code? Would be good to 
> know if this breaks anything.

Thanks for the heads up. I ran this on the same targets that broke in D84604 
 (in case that's what you're looking for), and 
those continue to pass. I can try further testing of other targets, but that 
may take a little longer, so I have no problem with you landing this as-is and 
I can follow up if there are problems elsewhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104261

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

[PATCH] D112732: [ASan] Process functions in Asan module pass

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:1189
+MPM.addPass(ModuleAddressSanitizerPass(
+CompileKernel, Recover, ModuleUseAfterScope, UseOdrIndicator,
+DestructorKind, UseAfterScope, UseAfterReturn));

I'm seeing build errors here, and I'm not sure what this is supposed to be?

```
/home/rupprecht/src/llvm-project/clang/lib/CodeGen/BackendUtil.cpp:1189:37: 
error: use of undeclared identifier 'ModuleUseAfterScope'; did you mean 
'UseAfterScope'?
CompileKernel, Recover, ModuleUseAfterScope, UseOdrIndicator,
^~~
UseAfterScope
```

https://buildkite.com/llvm-project/upstream-bazel-rbe/builds/11193#bbebbf99-a2c1-4959-b6f7-f7fb816591c0


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112732

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

[PATCH] D112732: [ASan] Process functions in Asan module pass

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:1189
+MPM.addPass(ModuleAddressSanitizerPass(
+CompileKernel, Recover, ModuleUseAfterScope, UseOdrIndicator,
+DestructorKind, UseAfterScope, UseAfterReturn));

rupprecht wrote:
> I'm seeing build errors here, and I'm not sure what this is supposed to be?
> 
> ```
> /home/rupprecht/src/llvm-project/clang/lib/CodeGen/BackendUtil.cpp:1189:37: 
> error: use of undeclared identifier 'ModuleUseAfterScope'; did you mean 
> 'UseAfterScope'?
> CompileKernel, Recover, ModuleUseAfterScope, UseOdrIndicator,
> ^~~
> UseAfterScope
> ```
> 
> https://buildkite.com/llvm-project/upstream-bazel-rbe/builds/11193#bbebbf99-a2c1-4959-b6f7-f7fb816591c0
I see it was reverted already, sorry!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112732

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

[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

I'm seeing a fair number of breakages from this patch (not really sure how many 
we truly have, I've hit ~5-10 so far in widely used libraries, but I suspect we 
have far more in the long tail). They're all valid (most are just adding 
missing thread safety annotations, etc., so far one breakage caught a real 
bug), but it would help if we could disable just these new ones. How 
invasive/annoying would it be to carve out a subwarning for this category of 
bugs and call it something like `-Wthread-safety-elision`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129755

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

[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D129755#3866213 , @aaronpuchert 
wrote:

> Are you seeing warnings because of the different treatment of copy-elided 
> construction, or because we've started to consider `CXXConstructorCall`s 
> outside of the initializer of a `DeclStmt`?

I'm not familiar with internal Clang APIs so I'm not entirely sure how to 
answer that, but I can share sketches of the breakages I've addressed so far:

1. (2x) Returning a MutexLock-like structure, e.g.

  MutexLock Lock() {
return MutexLock(&mu);
  }

this is documented in the test as a known false positive. Is that something you 
plan to address some day, or is it beyond the limits of thread safety analysis? 
(Or theoretically possible, but then compile times would be too much if you 
have to do more analysis?) Anyway, I applied 
`__attribute__((no_thread_safety_analysis))` to the method.

2. (3x) deferred/offloaded unlocking, e.g.

  void Func() {
auto *lock = new MutexLock(&mu);
auto cleanup = [lock]() { delete lock; };
cleanup();
  } // Error that mu is not unlocked

I assume this is beyond the scope of thread safety analysis -- `cleanup()` in 
the internal case actually happens in a different TU (the "cleanup" gets passed 
to some generic scheduler thingy), which is even more complex than this trivial 
example. In one of the cases the unlocking would happen on some other thread, 
which I guess is a little suspicious, but should still be guaranteed to execute 
(I didn't dig _too_ deep there). Anyway, I also suppressed analysis here.

3. Missed lock annotation on the constructor, take 1

  struct Foo {
explicit Foo(Mutex *mu) EXCLUSIVE_LOCKS_REQUIRED(mu);
  };
  
  Something Func() {
SomethingLikeMutexLock lock(&mu);
lock.Release(); // !!!
return Get(Foo(&mu));
  }

Glaringly obvious bug that wasn't being caught before this patch. Moving the 
call to `Foo(&mu)` earlier while the lock is still held fixes the bug.

4. Missed lock annotation on the constructor, take 2

  struct Foo {
explicit Foo(Mutex *mu) EXCLUSIVE_LOCKS_REQUIRED(mu);
  };
  void X() {
auto lock = MutexLock(&mu);
auto f = MakeFoo();
  }
  Foo Y() {
return Foo(&mu);
  }

`X` grabs the lock, but `Y` warns that we don't have `mu` held. In this case, 
`mu->AssertHeld()` suffices (the mutex pattern is a little more complicated, 
and the pattern is used elsewhere, but was missed in this method).

5. Alternate `std::make_unique` implementation (this one still has me puzzled)

  template 
  inline std::unique_ptr MyMakeUnique(Args &&...args) {
return std::unique_ptr(new T(std::forward(args)...));
  }
  
  static Mutex y_mu;
  
  class Z {
   public:
Z() EXCLUSIVE_LOCKS_REQUIRED(y_mu) {}
  };
  
  std::unique_ptr Y() {
auto lock = MutexLock(&y_mu);
return MyMakeUnique();
  }

Returning  `std::make_unique()` instead of the custom helper works. Why? No 
idea. `MyMakeUnique` is a c++11 compatibility helper -- std::unique_ptr is in 
c++11 but std::make_unique is only in c++14 -- and fortunately this code 
doesn't need it anymore. The implementation seems to be identical to the one in 
libc++, so my best guess is threading analysis has some special handling for 
some std:: methods.

I might have a better answer in a day or two of how widespread this is beyond 
just the core files that seem to make the world break when we enable this. We 
can just fix the bugs it if it's only a few of them, but it might be difficult 
if we have too many.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129755

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

[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D129755#3869081 , @aaronpuchert 
wrote:

> In D129755#3866887 , @rupprecht 
> wrote:
>
>> I might have a better answer in a day or two of how widespread this is 
>> beyond just the core files that seem to make the world break when we enable 
>> this. We can just fix the bugs it if it's only a few of them, but it might 
>> be difficult if we have too many.
>
> The good news is that for now we've only seen the second category of issues, 
> for which a flag to restore the old behavior would be feasible. I can't say 
> for certain whether that would make all the issues here disappear, but it 
> definitely seems so.

We're about done with our cleanup. Our fix count is at 34, and should be final 
unless there are surprises. I'm not sure if others would benefit from having 
this warning pushed to a subflag, but we don't need it anymore.

While making some fixes, I ran across a strange false positive which I filed as 
https://github.com/llvm/llvm-project/issues/58535. I think it's another 
situation where the code is too complex for thread analysis to be feasible, but 
I also didn't see it documented as one of the common cases that isn't supported.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129755

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

[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Sorry for the delay, I was out on vacation for a bit. I have a repro for this 
new issue now: F25778542: repro.tar.gz 

  $ CLANG=~/dev/clang ./repro.sh
  ++ dirname /tmp/repro/repro.sh
  + DIR=/tmp/repro
  + : /tmp/D136554
  + rm -rf /tmp/D136554
  + mkdir -p /tmp/D136554
  + cd /tmp/D136554
  + cp /tmp/repro/lib.h /tmp/repro/outer.h /tmp/repro/x.h /tmp/repro/y.h 
/tmp/repro/z.h /tmp/repro/lib.cppmap /tmp/repro/outer.cppmap 
/tmp/repro/x.cppmap /tmp/repro/y.cppmap /tmp/repro/z.cppmap .
  + COMMON_ARGS=('-Wno-mismatched-tags' '-Xclang=-fmodules-embed-all-files' 
'-Xclang=-fmodules-local-submodule-visibility' '-fmodules' 
'-fno-implicit-modules' '-fno-implicit-module-maps' '-std=gnu++17' 
'-Xclang=-fmodule-map-file-home-is-cwd')
  + export COMMON_ARGS
  + /home/rupprecht/dev/clang -Wno-mismatched-tags 
-Xclang=-fmodules-embed-all-files -Xclang=-fmodules-local-submodule-visibility 
-fmodules -fno-implicit-modules -fno-implicit-module-maps -std=gnu++17 
-Xclang=-fmodule-map-file-home-is-cwd -fmodule-name=x 
-fmodule-map-file=x.cppmap -fmodule-map-file=lib.cppmap -xc++ 
-Xclang=-emit-module -c x.cppmap -o x.pcm
  + /home/rupprecht/dev/clang -Wno-mismatched-tags 
-Xclang=-fmodules-embed-all-files -Xclang=-fmodules-local-submodule-visibility 
-fmodules -fno-implicit-modules -fno-implicit-module-maps -std=gnu++17 
-Xclang=-fmodule-map-file-home-is-cwd -fmodule-name=y 
-fmodule-map-file=y.cppmap -fmodule-map-file=lib.cppmap 
-fmodule-map-file=z.cppmap -xc++ -Xclang=-emit-module -c y.cppmap -o y.pcm
  + /home/rupprecht/dev/clang -Wno-mismatched-tags 
-Xclang=-fmodules-embed-all-files -Xclang=-fmodules-local-submodule-visibility 
-fmodules -fno-implicit-modules -fno-implicit-module-maps -std=gnu++17 
-Xclang=-fmodule-map-file-home-is-cwd -fmodule-name=outer 
-fmodule-map-file=outer.cppmap -Xclang=-fmodule-file=x.pcm 
-Xclang=-fmodule-file=y.pcm -fmodule-map-file=lib.cppmap 
-fmodule-map-file=x.cppmap -fmodule-map-file=y.cppmap -xc++-header 
-fsyntax-only -c outer.h -o outer.h.processed
  In module 'x':
  ./lib.h:5:25: error: 'Foo' has different definitions in different modules; 
first difference is definition in module 'x.x.h' found data member 'kConstant' 
with an initializer
static constexpr char kConstant = '+';
~~^~~
  ./lib.h:5:25: note: but in 'y.y.h' found data member 'kConstant' with a 
different initializer
static constexpr char kConstant = '+';
~~^~~
  1 error generated.



In D136554#4000654 , @cor3ntin wrote:

> In D136554#4000363 , @rupprecht 
> wrote:
>
>> I applied this version of the patch and the crash is now gone 🎉
>>
>> However, now I get this inexplicable error -- I'm not entirely sure it's 
>> related, maybe I'm holding it wrong:
>>
>>   In module '':
>>   foo.h$line:$num: error: 'foo::FooClass' has different definitions in 
>> different modules; first difference is definition in module 'something.h' 
>> found data member 'kFooDelimiter' with an initializer
>> static constexpr char kFooDelimiter = '+';
>> ~~^~~
>>   foo.h:$line:$num note: but in 'other.h' found data member 'kFooDelimiter' 
>> with a different initializer
>> static constexpr char kFooDelimiter = '+';
>> ~~^~~
>>
>> The definition seems straightforward:
>>
>>   class FooClass final {
>> ...
>> static constexpr char kFooDelimiter = '+';
>> ...
>>   };
>
> This is *very* surprising to me.
> I could explain it if  the member was not static though, as it would be the 
> kind of things the patch affects. But static data members are handled very 
> differently.
>
> Was that liking chrome? It didn't come up in my tests

No, it's some other internal target. There isn't any way for you to be able to 
test against these targets, so unfortunately the best I can offer is I'll patch 
in any updated versions of this patch, see what breaks, and try to reduce it 
when reporting.

There's barely any code in the repro I attached, it's just a bunch of 
header/module layering. My guess is that clang is usually able to see that the 
two definitions in different modules is the same and therefore dedupes them, 
but this change adds some unique bit of information that makes clang think it's 
different. Note that the headers need to be in a particular order to break.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554

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

[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Glad the test case made sense to you, it was convoluted to me :)

Still seeing one more error, and it's not modules-related so I might be able to 
get it reduced today. Generally, it looks like this:

  struct Inner {
Foo& foo;
const std::unique_ptr<...> x = blah(blah(
&foo.bar()));
  };
  
  class Outer {
   private:
Foo foo_;
Inner inner{foo_};
  }

With the error being:

  error: 'Inner::foo' is not a member of class 'Outer'
&foo.bar()));

I think this build failure is wrong? `foo` should be referring to the 
definition inside `Inner`, but clang seems to be expecting it to refer to 
something in `Outer`.

Is it expected that this patch will cause some previously "working" code to no 
longer build? At some point I expect to hand you a reduction that's actually a 
bug in the user code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554

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

[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D136554#4013463 , @rupprecht wrote:

> Glad the test case made sense to you, it was convoluted to me :)
>
> Still seeing one more error, and it's not modules-related so I might be able 
> to get it reduced today. Generally, it looks like this:
>
>   struct Inner {
> Foo& foo;
> const std::unique_ptr<...> x = blah(blah(
> &foo.bar()));
>   };
>   
>   class Outer {
>private:
> Foo foo_;
> Inner inner{foo_};
>   }
>
> With the error being:
>
>   error: 'Inner::foo' is not a member of class 'Outer'
> &foo.bar()));
>
> I think this build failure is wrong? `foo` should be referring to the 
> definition inside `Inner`, but clang seems to be expecting it to refer to 
> something in `Outer`.
>
> Is it expected that this patch will cause some previously "working" code to 
> no longer build? At some point I expect to hand you a reduction that's 
> actually a bug in the user code.

Fully reduced as:

  template  int c(a, b);
  struct d {
static d e(const char * = __builtin_FILE());
  };
  struct f {
f(d = d::e());
  };
  struct h {
int &g;
int blah = c(g, f());
  };
  struct k {
k();
int i;
h j{i};
  };
  k::k() {}

With the error: `repro.cc:10:16: error: 'h::g' is not a member of class 'k'`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554

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

[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

All good now! The latest revision of this patch doesn't seem to break anything, 
unless I ran our tests wrong. From my perspective this is OK to reland now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554

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

[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D136554#4018870 , @rupprecht wrote:

> All good now! The latest revision of this patch doesn't seem to break 
> anything, unless I ran our tests wrong. From my perspective this is OK to 
> reland now.

... and yep, I was holding it wrong. There are still build failures, this time 
in linking, where LLD reports some symbols are undefined. I'm not certain yet 
if this really a problem with this patch (e.g. sometimes we see files break 
like this when users put their template code in the .cpp files and not the .h 
files), but the source code doesn't look immediately wrong. Once again, I'll 
try to reduce something.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554

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

[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

I'm not sure what to make of the new failure when I try it out this time. Given 
a source like this:

  #include 
  
  struct Options {
std::function identity = [](bool x) -> bool { return x; };
  };
  
  struct Wrapper {
explicit Wrapper(const Options& options = Options()) {}
  };
  
  void Func() { Options options; }

The object file reports many fewer symbols as a result of this patch, but one 
new problematic one. Before:

   T Func()
   W 
std::__1::__function::__func, bool 
(bool)>::target_type() const
   W 
std::__1::__function::__func, bool 
(bool)>::target(std::type_info const&) const
   W 
std::__1::__function::__func, bool 
(bool)>::__clone(std::__1::__function::__base*) const
   W 
std::__1::__function::__func, bool (bool)>::__clone() 
const
   W std::__1::__function::__base::~__base[abi:v16]()
   W 
std::__1::__function::__func, bool 
(bool)>::destroy_deallocate()
   W 
std::__1::__function::__func, bool (bool)>::destroy()
   W 
std::__1::__function::__func, bool (bool)>::~__func()
   W 
std::__1::__function::__func, bool 
(bool)>::operator()(bool&&)
   V typeinfo for Options::identity::'lambda'(bool)
   V typeinfo for std::__1::__function::__base
   V typeinfo for 
std::__1::__function::__func, bool (bool)>
   V typeinfo name for Options::identity::'lambda'(bool)
   V typeinfo name for std::__1::__function::__base
   V typeinfo name for 
std::__1::__function::__func, bool (bool)>
   U vtable for __cxxabiv1::__class_type_info
   U vtable for __cxxabiv1::__si_class_type_info
   V vtable for 
std::__1::__function::__func, bool (bool)>
   U operator delete(void*)
   U operator new(unsigned long)

And after:

   T Func()
   U std::__1::function::function(Options::identity::'lambda'(bool))

The undefined symbols before are all provided by libc++, so those are fine. 
After, the new undefined symbol for the lambda cannot be resolved. Depending on 
how the linker is invoked, this may or may not be fine -- IIUC the linker can 
sometimes recognize that it doesn't actually need the undefined symbol, so it 
doesn't matter if it can't find it.

Here is the build script I'm using, although you will likely need to tweak it 
for your own environment:

  ${CLANG} \
-std=gnu++17 -stdlib=libc++ \
-c main.cc \
-o /tmp/main.o
  
  echo "main.o symbols:"
  llvm-nm -C /tmp/main.o
  
  echo Building b.cc
  
  ${CLANG} \
-std=gnu++17 -stdlib=libc++ \
-O1 -fno-exceptions \
-c b.cc \
-o /tmp/b.o
  
  echo "b.o symbols:"
  llvm-nm -C /tmp/b.o
  
  echo Linking, and expecting success
  
  ${CLANG} \
-fuse-ld=lld \
-o /tmp/main \
/tmp/main.o \
-Wl,--start-lib /tmp/b.o -Wl,--end-lib \
/usr/lib/x86_64-linux-gnu/libc++.so
  
  echo Linking, and expecting failure with D136554
  
  ${CLANG} \
-fuse-ld=lld \
-o /tmp/main \
/tmp/main.o \
/tmp/b.o \
/usr/lib/x86_64-linux-gnu/libc++.so \
-Wl,--export-dynamic

(Here `b.cc` is the first snippet above, and `main.cc` is a trivial file with 
just `int main(int argc, char* argv[]) { return 0; }`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554

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

[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

I threw this at the "test everything" test (some millions of targets) and it 
found only one breakage, so this is very very close. Without further ado, here 
is this silly looking thing:

File `blah.h`:

  #include 
  #include 
  
  template 
  struct Base {
virtual ~Base() = default;
  };
  
  class Impl : public Base {};
  
  struct ImplHolder {
std::unique_ptr> impl = std::make_unique();
  };
  
  struct GetImplCreators {
std::function impl_creator = []() { return ImplHolder{}; };
  };
  
  void UseImpl(GetImplCreators impl_creators = GetImplCreators{});

And `blah.cc`:

  #include "blah.h"
  
  void UseImpl(GetImplCreators impl_creators) {}

And lastly, `blah_main.cc`:

  #include "blah.h"
  
  void Func1() { UseImpl(); }
  // Note: it also fails when we explicitly specify the default arg, in case 
that is interesting.
  // void Func2() { UseImpl(GetImplCreators()); }
  
  int main(int argc, char* argv[]) { return 0; }

And here's the LLD output:

  ld: error: undefined hidden symbol: 
std::__u::__unique_if::__unique_single std::__u::make_unique()
  >>> referenced by blah_main.cc
  >>>   blah_main.o:(ImplHolder 
std::__u::__function::__policy_invoker::__call_impl>(std::__u::__function::__policy_storage const*))
  
  ld: error: undefined hidden symbol: std::__u::unique_ptr>::~unique_ptr()
  >>> referenced by blah_main.cc
  >>>   blah_main.o:(ImplHolder 
std::__u::__function::__policy_invoker::__call_impl>(std::__u::__function::__policy_storage const*))

I'm out of time today so I don't have a repro script to offer, but I can dig 
into that when I get back next year if the source-only example is insufficient.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554

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

[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht accepted this revision.
rupprecht added a comment.

I still see one behavior change (actually it was there before, but I missed it 
in the test results), but as far as I can tell, it's a good one? If I reduce it 
too much, I get the warning with the baseline toolchain, so it's not erroneous 
AFAICT. Although I won't pretend I know all the intricacies of `static` and 
`inline`.

  // a.h
  static const std::pair& GetFakePair() {
static constexpr std::pair kFakePair = {123.0, 456.0};
return kFakePair;
  }
  
  // b.h
  #include "a.h"
  
  class X {
void Foo(..., const std::pair& x = GetFakePair()) const;
  };
  
  // main.cc, compiled with -Wunused-function
  #include "b.h"
  int main() { return 0; }

Yields this error:

  In file included from main.cc:9:
  In file included from ./b.h:16:
  ./a.h:40:41: error: 'static' function 'GetFakePair' declared in header file 
should be declared 'static inline' [-Werror,-Wunneeded-internal-declaration]
  static const std::pair& GetFakePair() {

Unless I'm missing something, it seems that, in regards to this warning, this 
change is just enabling an existing warning to have more coverage, and 
therefore this change looks ready to ship. Thanks for letting me provide all my 
repros before landing this one again :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554

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

[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D136554#4024628 , @rupprecht wrote:

> I still see one behavior change (actually it was there before, but I missed 
> it in the test results), but as far as I can tell, it's a good one? If I 
> reduce it too much, I get the warning with the baseline toolchain, so it's 
> not erroneous AFAICT. Although I won't pretend I know all the intricacies of 
> `static` and `inline`.

I missed another place in the unreduced repro that was referencing this method, 
and that was the trick. I reduced the newly-enabled warning case to this:

  // a.h
  static const std::pair& GetFakePairRef() {
static constexpr std::pair fake = {1.0, 2.0};
return fake;
  }
  
  struct Metadata {
const std::pair& data = GetFakePairRef();
  };
  
  // main.cc, compiled with -Wunused-function
  #include "a.h"
  int main() { return 0; }

Again, still LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554

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

[PATCH] D142449: [clang] Fix linking to LLVMTestingAnnotations in standalone build

[Jordan Rupprecht via Phabricator via cfe-commits]

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

Thanks!


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

https://reviews.llvm.org/D142449

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

[PATCH] D124351: [Clang] Implement Change scope of lambda trailing-return-type

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Hi, me again :)

I ran into an interesting build breakage from this, I can't tell if it's a 
legitimate breakage based on reading P2036R3 and P2579R0 (mostly I'm not a 
language lawyer).

  struct StringLiteral {
template 
StringLiteral(const char (&array)[N])
__attribute__((enable_if(N > 0 && N == __builtin_strlen(array) + 1,
 "invalid string literal")));
  };
  
  struct Message {
Message(StringLiteral);
  };
  
  void Func1() {
auto x = Message("x");  // Note: this is fine
  
// Note: "xx\0" to force a different type, StringLiteral<3>, otherwise this
// successfully builds.
auto y = [&](decltype(Message("xx"))) {};
  
// ^ fails with: repro.cc:18:13: error: reference to local variable 'array'
// declared in enclosing function 'StringLiteral::StringLiteral<3>'
  
(void)x;
(void)y;
  }

https://godbolt.org/z/M4E6jKxxn

Does this look like an intended breakage from this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124351

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

[PATCH] D124351: [Clang] Implement Change scope of lambda trailing-return-type

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D124351#4102679 , @aaron.ballman 
wrote:

> In D124351#4102634 , @eaeltsin 
> wrote:
>
>>> Looks like we fail to enter the appropriate context somewhere (my guess is 
>>> that it might be specific to the attribute but it's hard to say without 
>>> picking around), definitely a bug
>>>
>>> I'll be away the next 2 weeks, I'll look at it when I get back. Maybe we 
>>> should track it in an issue though.
>>
>> Should we revert this then?
>
> Yes, I think we should. We should also file an issue so @cor3ntin doesn't 
> lose track of the issue. Anyone want to volunteer to do that? (I can do it 
> early next week otherwise.)

Done in 74ce297045bac4bc475b8e762d2a1ea19bb16d3c 
 and filed 
https://github.com/llvm/llvm-project/issues/60518 to track re-landing this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124351

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

[PATCH] D141175: [bazel] Split out Annotations from `TestingSupport`

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht updated this revision to Diff 488366.
rupprecht added a comment.
Herald added subscribers: cfe-commits, kadircet, arphaman, hiraditya.
Herald added projects: clang, clang-tools-extra.

- Move annotations to a separate package entirely


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141175

Files:
  clang-tools-extra/clangd/unittests/Annotations.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
  clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
  clang-tools-extra/include-cleaner/unittests/CMakeLists.txt
  clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp
  clang-tools-extra/include-cleaner/unittests/LocateSymbolTest.cpp
  clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
  clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
  clang-tools-extra/pseudo/unittests/BracketTest.cpp
  clang-tools-extra/pseudo/unittests/CMakeLists.txt
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
  clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
  clang/docs/tools/clang-formatted-files.txt
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/DeclTest.cpp
  clang/unittests/AST/SourceLocationTest.cpp
  clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
  clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp
  clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
  clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
  clang/unittests/Analysis/FlowSensitive/TestingSupport.h
  clang/unittests/Analysis/FlowSensitive/TransferBranchTest.cpp
  clang/unittests/Sema/CMakeLists.txt
  clang/unittests/Sema/CodeCompleteTest.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/SourceCodeTest.cpp
  clang/unittests/Tooling/Syntax/CMakeLists.txt
  clang/unittests/Tooling/Syntax/TokensTest.cpp
  clang/unittests/Tooling/Syntax/TreeTestBase.cpp
  clang/unittests/Tooling/Syntax/TreeTestBase.h
  llvm/include/llvm/Testing/Annotations/Annotations.h
  llvm/include/llvm/Testing/Support/Annotations.h
  llvm/lib/Testing/Annotations/Annotations.cpp
  llvm/lib/Testing/Annotations/CMakeLists.txt
  llvm/lib/Testing/CMakeLists.txt
  llvm/lib/Testing/Support/Annotations.cpp
  llvm/lib/Testing/Support/CMakeLists.txt
  llvm/unittests/Support/AnnotationsTest.cpp
  llvm/unittests/Support/CMakeLists.txt
  llvm/unittests/Testing/Annotations/AnnotationsTest.cpp
  llvm/unittests/Testing/Annotations/CMakeLists.txt
  llvm/unittests/Testing/CMakeLists.txt
  utils/bazel/llvm-project-overlay/llvm/BUILD.bazel

Index: utils/bazel/llvm-project-overlay/llvm/BUILD.bazel
===
--- utils/bazel/llvm-project-overlay/llvm/BUILD.bazel
+++ utils/bazel/llvm-project-overlay/llvm/BUILD.bazel
@@ -4450,18 +4450,27 @@
 "lib/Testing/Support/*.cpp",
 "lib/Testing/Support/*.h",
 ]),
-hdrs = glob([
-"include/llvm/Testing/Support/*.h",
-]),
+hdrs = glob(["include/llvm/Testing/Support/*.h"]),
 copts = llvm_copts,
 deps = [
 ":Support",
+":TestingAnnotations",
 ":config",
 "//third-party/unittest:gmock",
 "//third-party/unittest:gtest",
 ],
 )
 
+# Split out of TestingSupport to allow using this with a different gmock/gtest version.
+cc_library(
+name = "TestingAnnotations",
+testonly = True,
+srcs = ["lib/Testing/Annotations/Annotations.cpp"],
+hdrs = ["include/llvm/Testing/Annotations/Annotations.h"],
+copts = llvm_copts,
+deps = [":Support"],
+)
+
 
 # Begin testonly binary utilities
 
Index: llvm/unittests/Testing/CMakeLists.txt
===
--- llvm/unittests/Testing/CMakeLists.txt
+++ llvm/unittests/Testing/CMakeLists.txt
@@ -1,2 +1,3 @@
 add_subdirectory(ADT)
+add_subdirectory(Annotations)
 add_subdirectory(Support)
Index: llvm/unittests/Testing/Annotations/CMakeLists.txt
===
--- /dev/null
+++ llvm/unittests/Testing/Annotations/CMakeLists.txt
@@ -0,0 +1,10 @@
+set(LLVM_LINK_COMPONENTS
+  Support
+  TestingAnnotations
+  )
+
+add_llvm_unittest(TestingAnnotationTests
+  AnnotationsTest.cpp
+  )
+
+target_link_libraries(TestingAnnotationTests PRIVATE LLVMTestingAnnotations)
Index: llvm/unittests/Testing/Annotations/AnnotationsTest.cpp
===
--- llvm/unittests/Testing/Annotations/AnnotationsTest.cpp
+++ llvm/unittests/Testing/Annotations/AnnotationsTest.cpp
@@ -5,7 +5,7 @@
 // SPDX-License-Identifier: Apache-2.0 WITH

[PATCH] D141175: [bazel] Split out Annotations from `TestingSupport`

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D141175#4038017 , @GMNGeoffrey 
wrote:

> It seems like the same logic would extend to the CMake build. Could we make 
> the same change there?

The simplest (only?) way to do that is to have it literally in a separate 
directory, so I did that. It's a bit large now but mechanical.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141175

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

[PATCH] D141175: [test] Split out Annotations from `TestingSupport`

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht updated this revision to Diff 488381.
rupprecht added a comment.

- Remove redundant comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141175

Files:
  clang-tools-extra/clangd/unittests/Annotations.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
  clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
  clang-tools-extra/include-cleaner/unittests/CMakeLists.txt
  clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp
  clang-tools-extra/include-cleaner/unittests/LocateSymbolTest.cpp
  clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
  clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
  clang-tools-extra/pseudo/unittests/BracketTest.cpp
  clang-tools-extra/pseudo/unittests/CMakeLists.txt
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
  clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
  clang/docs/tools/clang-formatted-files.txt
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/DeclTest.cpp
  clang/unittests/AST/SourceLocationTest.cpp
  clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
  clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp
  clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
  clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
  clang/unittests/Analysis/FlowSensitive/TestingSupport.h
  clang/unittests/Analysis/FlowSensitive/TransferBranchTest.cpp
  clang/unittests/Sema/CMakeLists.txt
  clang/unittests/Sema/CodeCompleteTest.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/SourceCodeTest.cpp
  clang/unittests/Tooling/Syntax/CMakeLists.txt
  clang/unittests/Tooling/Syntax/TokensTest.cpp
  clang/unittests/Tooling/Syntax/TreeTestBase.cpp
  clang/unittests/Tooling/Syntax/TreeTestBase.h
  llvm/include/llvm/Testing/Annotations/Annotations.h
  llvm/include/llvm/Testing/Support/Annotations.h
  llvm/lib/Testing/Annotations/Annotations.cpp
  llvm/lib/Testing/Annotations/CMakeLists.txt
  llvm/lib/Testing/CMakeLists.txt
  llvm/lib/Testing/Support/Annotations.cpp
  llvm/lib/Testing/Support/CMakeLists.txt
  llvm/unittests/Support/AnnotationsTest.cpp
  llvm/unittests/Support/CMakeLists.txt
  llvm/unittests/Testing/Annotations/AnnotationsTest.cpp
  llvm/unittests/Testing/Annotations/CMakeLists.txt
  llvm/unittests/Testing/CMakeLists.txt
  utils/bazel/llvm-project-overlay/llvm/BUILD.bazel

Index: utils/bazel/llvm-project-overlay/llvm/BUILD.bazel
===
--- utils/bazel/llvm-project-overlay/llvm/BUILD.bazel
+++ utils/bazel/llvm-project-overlay/llvm/BUILD.bazel
@@ -4450,18 +4450,26 @@
 "lib/Testing/Support/*.cpp",
 "lib/Testing/Support/*.h",
 ]),
-hdrs = glob([
-"include/llvm/Testing/Support/*.h",
-]),
+hdrs = glob(["include/llvm/Testing/Support/*.h"]),
 copts = llvm_copts,
 deps = [
 ":Support",
+":TestingAnnotations",
 ":config",
 "//third-party/unittest:gmock",
 "//third-party/unittest:gtest",
 ],
 )
 
+cc_library(
+name = "TestingAnnotations",
+testonly = True,
+srcs = ["lib/Testing/Annotations/Annotations.cpp"],
+hdrs = ["include/llvm/Testing/Annotations/Annotations.h"],
+copts = llvm_copts,
+deps = [":Support"],
+)
+
 
 # Begin testonly binary utilities
 
Index: llvm/unittests/Testing/CMakeLists.txt
===
--- llvm/unittests/Testing/CMakeLists.txt
+++ llvm/unittests/Testing/CMakeLists.txt
@@ -1,2 +1,3 @@
 add_subdirectory(ADT)
+add_subdirectory(Annotations)
 add_subdirectory(Support)
Index: llvm/unittests/Testing/Annotations/CMakeLists.txt
===
--- /dev/null
+++ llvm/unittests/Testing/Annotations/CMakeLists.txt
@@ -0,0 +1,10 @@
+set(LLVM_LINK_COMPONENTS
+  Support
+  TestingAnnotations
+  )
+
+add_llvm_unittest(TestingAnnotationTests
+  AnnotationsTest.cpp
+  )
+
+target_link_libraries(TestingAnnotationTests PRIVATE LLVMTestingAnnotations)
Index: llvm/unittests/Testing/Annotations/AnnotationsTest.cpp
===
--- llvm/unittests/Testing/Annotations/AnnotationsTest.cpp
+++ llvm/unittests/Testing/Annotations/AnnotationsTest.cpp
@@ -5,7 +5,7 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
-#include "llvm/Testing/Support/Annotations.h"
+#include "llvm/Testing/Annotations/Annotations.h"
 #include "gmock/gmock.h"
 #inc

[PATCH] D141175: [test] Split out Annotations from `TestingSupport`

[Jordan Rupprecht via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3432f4bf86e7: [test] Split out Annotations from 
`TestingSupport` (authored by rupprecht).

Changed prior to commit:
  https://reviews.llvm.org/D141175?vs=488381&id=488762#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141175

Files:
  clang-tools-extra/clangd/unittests/Annotations.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h
  clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
  clang-tools-extra/include-cleaner/unittests/CMakeLists.txt
  clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp
  clang-tools-extra/include-cleaner/unittests/LocateSymbolTest.cpp
  clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
  clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
  clang-tools-extra/pseudo/unittests/BracketTest.cpp
  clang-tools-extra/pseudo/unittests/CMakeLists.txt
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
  clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
  clang/docs/tools/clang-formatted-files.txt
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/DeclTest.cpp
  clang/unittests/AST/SourceLocationTest.cpp
  clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
  clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp
  clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
  clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp
  clang/unittests/Analysis/FlowSensitive/TestingSupport.h
  clang/unittests/Analysis/FlowSensitive/TransferBranchTest.cpp
  clang/unittests/Sema/CMakeLists.txt
  clang/unittests/Sema/CodeCompleteTest.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/SourceCodeTest.cpp
  clang/unittests/Tooling/Syntax/CMakeLists.txt
  clang/unittests/Tooling/Syntax/TokensTest.cpp
  clang/unittests/Tooling/Syntax/TreeTestBase.cpp
  clang/unittests/Tooling/Syntax/TreeTestBase.h
  llvm/include/llvm/Testing/Annotations/Annotations.h
  llvm/include/llvm/Testing/Support/Annotations.h
  llvm/lib/Testing/Annotations/Annotations.cpp
  llvm/lib/Testing/Annotations/CMakeLists.txt
  llvm/lib/Testing/CMakeLists.txt
  llvm/lib/Testing/Support/Annotations.cpp
  llvm/lib/Testing/Support/CMakeLists.txt
  llvm/unittests/Support/AnnotationsTest.cpp
  llvm/unittests/Support/CMakeLists.txt
  llvm/unittests/Testing/Annotations/AnnotationsTest.cpp
  llvm/unittests/Testing/Annotations/CMakeLists.txt
  llvm/unittests/Testing/CMakeLists.txt
  utils/bazel/llvm-project-overlay/clang/unittests/BUILD.bazel
  utils/bazel/llvm-project-overlay/llvm/BUILD.bazel

Index: utils/bazel/llvm-project-overlay/llvm/BUILD.bazel
===
--- utils/bazel/llvm-project-overlay/llvm/BUILD.bazel
+++ utils/bazel/llvm-project-overlay/llvm/BUILD.bazel
@@ -4516,9 +4516,7 @@
 "lib/Testing/Support/*.cpp",
 "lib/Testing/Support/*.h",
 ]),
-hdrs = glob([
-"include/llvm/Testing/Support/*.h",
-]),
+hdrs = glob(["include/llvm/Testing/Support/*.h"]),
 copts = llvm_copts,
 deps = [
 ":Support",
@@ -4528,6 +4526,15 @@
 ],
 )
 
+cc_library(
+name = "TestingAnnotations",
+testonly = True,
+srcs = ["lib/Testing/Annotations/Annotations.cpp"],
+hdrs = ["include/llvm/Testing/Annotations/Annotations.h"],
+copts = llvm_copts,
+deps = [":Support"],
+)
+
 
 # Begin testonly binary utilities
 
Index: utils/bazel/llvm-project-overlay/clang/unittests/BUILD.bazel
===
--- utils/bazel/llvm-project-overlay/clang/unittests/BUILD.bazel
+++ utils/bazel/llvm-project-overlay/clang/unittests/BUILD.bazel
@@ -29,6 +29,7 @@
 "//clang:tooling",
 "//llvm:Core",
 "//llvm:Support",
+"//llvm:TestingAnnotations",
 "//llvm:TestingSupport",
 "//third-party/unittest:gmock",
 "//third-party/unittest:gtest",
@@ -136,6 +137,7 @@
 "//clang:tooling",
 "//llvm:Support",
 "//llvm:TestingADT",
+"//llvm:TestingAnnotations",
 "//llvm:TestingSupport",
 "//third-party/unittest:gmock",
 "//third-party/unittest:gtest",
@@ -342,6 +344,7 @@
 "//clang:parse",
 "//clang:sema",
 "//clang:tooling",
+"//llvm:TestingAnnotations",
 "//llvm:TestingSupport",
 "//third-party/unittest:gmock",
 "//third-party/unittest:gtest",
@@ -420,6 +423,7 @@
 "//clang:tooling_refactoring",

[PATCH] D153156: [Clang] CWG1473: do not err on the lack of space after operator""

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D153156#4599106 , @aaron.ballman 
wrote:

> In D153156#4598988 , @rZhBoYao 
> wrote:
>
>> In D153156#4598915 , 
>> @steelannelida wrote:
>>
>>> Unfortunately the option -Wno-reserved-user-defined-literal fails after 
>>> this:
>>>
>>>   #define MYTHING "_something_"
>>>   
>>>   const char* f() {
>>> return "ONE"MYTHING"TWO";
>>>   }
>>>
>>>   $ clang -Wno-reserved-user-defined-literal repro.cxx
>>>   repro.cxx:4:15: error: no matching literal operator for call to 
>>> 'operator""MYTHING' with arguments of types 'const char *' and 'unsigned 
>>> long', and no matching literal operator template
>>>   4 |   return "ONE"MYTHING"TWO";
>>> |   ^
>>>   1 error generated.
>>
>> This is conforming right? Correct me if I'm wrong. My reading of 
>> https://eel.is/c++draft/lex.pptoken#3.3 is that "ONE"MYTHING"TWO" is a 
>> single preprocessing-token during phase 3 
>> (https://eel.is/c++draft/lex.phases#1.3). Can @aaron.ballman confirm this?
>
> The diagnostic behavior is correct. `MYTHING` doesn't get expanded until 
> phase 4 (http://eel.is/c++draft/lex.phases#1.4), so this appears as 
> `"ONE"MYTHING` as a single preprocessor token: 
> https://eel.is/c++draft/lex.ext#nt:user-defined-string-literal and that token 
> is an invalid UDL.

IIUC, the question is not whether the diagnostic is correct, but rather why 
`-Wno-reserved-user-defined-literal` does not workaround the breakage. Is that 
right?

An example of this in the wild is older versions of swig: 
https://github.com/swig/swig/blob/939dd5e1c8c17e5f8b38747bf18e9041ab5f377e/Source/Modules/php.cxx#L1724


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153156

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

[PATCH] D153156: [Clang] CWG1473: do not err on the lack of space after operator""

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

The other common breakage I'm seeing is code that hasn't yet migrated from the 
"PRI" format macros, of which there's an example in LLVM itself: 
https://github.com/llvm/llvm-project/blob/6a0e536ccfef1f7bd64ee4153b4efc0aeecf28d4/clang/test/SemaCXX/cxx11-compat.cpp#L38

e.g.: https://godbolt.org/z/v3boc9E6T

> IMHO, people should stop using -Wno-reserved-user-defined-literal and 
> exploiting the addition of a whitespace to mingle pre-c++11 and post-c++11 
> code.

I mostly agree with this, at least the spirit of it, but do we usually remove 
the `-Wno` ability to suppress a common error because we don't like it? (Or 
justify it post-facto if we accidentally did that?) If removing this is 
important for some other aspect of clang development, it would be nice to have 
notice ahead of time that this option will go away. I guess +1 to what James 
said.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153156

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

[PATCH] D153156: [Clang] CWG1473: do not err on the lack of space after operator""

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D153156#4600270 , @rupprecht wrote:

> of which there's an example in LLVM itself: 
> https://github.com/llvm/llvm-project/blob/6a0e536ccfef1f7bd64ee4153b4efc0aeecf28d4/clang/test/SemaCXX/cxx11-compat.cpp#L38

Sorry, I don't want to mislead: I just mean there's a handy example in clang's 
unit tests. I don't see any instances of this in LLVM non-test code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153156

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

[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:13047
+  F->setUsesFPIntrin(true);
+  printf("Enclosing function uses fp intrinsics\n");
+}

Looks like this is leftover debugging? I'm seeing log spam compiling some files 
-- this message repeated hundreds of times. I'll go ahead and create a patch 
that nukes this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62731



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

[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Jordan Rupprecht via Phabricator via cfe-commits]

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



Comment at: clang/lib/Sema/SemaExpr.cpp:13047
+  F->setUsesFPIntrin(true);
+  printf("Enclosing function uses fp intrinsics\n");
+}

rupprecht wrote:
> Looks like this is leftover debugging? I'm seeing log spam compiling some 
> files -- this message repeated hundreds of times. I'll go ahead and create a 
> patch that nukes this.
Sorry for the noise, looks like f4a7d5659df7cb56c1baa34a39e9fe2639472741 
already did this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62731



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

[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:444
 
+def warn_drv_experimental_fp_control_incomplete_opt : Warning<
+  "Support for floating point control option %0 is incomplete and 
experimental">,

mibintc wrote:
> @kpn thought it would be a good idea to add a Warning that the implementation 
> of float control is experimental and partially implemented.  That's what this 
> is for.
Instead of adding a warning, I'd like to propose `-frounding-math` not be 
enabled unless an additional flag (e.g. `-fexperimental-float-control`) is 
passed. Or maybe this feature should be called 
`-f[no-]experimental-rounding-math` instead.

There are plenty of builds that are already specifying `-frounding-math` (e.g. 
they also support building w/ a compiler such as gcc that implements this), and 
adding this experimental/incomplete implementation is a surprise to those 
builds.

If I'm wrong and it's completely safe to ignore the warning (maybe it's 
incomplete but not in any unsafe way), we should just not have it at all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62731



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

[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:444
 
+def warn_drv_experimental_fp_control_incomplete_opt : Warning<
+  "Support for floating point control option %0 is incomplete and 
experimental">,

andrew.w.kaylor wrote:
> rupprecht wrote:
> > mibintc wrote:
> > > @kpn thought it would be a good idea to add a Warning that the 
> > > implementation of float control is experimental and partially 
> > > implemented.  That's what this is for.
> > Instead of adding a warning, I'd like to propose `-frounding-math` not be 
> > enabled unless an additional flag (e.g. `-fexperimental-float-control`) is 
> > passed. Or maybe this feature should be called 
> > `-f[no-]experimental-rounding-math` instead.
> > 
> > There are plenty of builds that are already specifying `-frounding-math` 
> > (e.g. they also support building w/ a compiler such as gcc that implements 
> > this), and adding this experimental/incomplete implementation is a surprise 
> > to those builds.
> > 
> > If I'm wrong and it's completely safe to ignore the warning (maybe it's 
> > incomplete but not in any unsafe way), we should just not have it at all.
> You raise an interesting point about people who might be using 
> -frounding-math already. However, if they are using this flag because they 
> also sometimes build with a compiler such as gcc that supports the flag, they 
> are currently getting incorrect behavior from clang. Without this patch, 
> clang silently ignores the option and the optimizer silently ignores the fact 
> that the program may be changing the rounding mode dynamically. The user may 
> or may not be aware of this.
> 
> With this patch such a user is likely to observe two effects: (1) their code 
> will suddenly get slower, and (2) it will probably start behaving correctly 
> with regard to rounding mode changes. The rounding behavior will definitely 
> not get worse. I think the warning is useful as an indication that something 
> has changed. I don't think requiring an additional option should be necessary.
> However, if they are using this flag because they also sometimes build with a 
> compiler such as gcc that supports the flag, they are currently getting 
> incorrect behavior from clang

Incorrect, yes, but also likely valid behavior. "experimental" seems to imply a 
miscompile when using this option should not be unexpected.

As I suggested before: if I'm wrong, and this behavior is only going to make 
the code more correct (as you suggest), can we remove the warning that this 
must be ack'd explicitly by adding `-Wno-experimental-float-control` to builds? 
I don't understand the motivation for the warning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62731



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

[PATCH] D71554: [llvm-ranlib] Handle -D and -U command line flag

[Jordan Rupprecht via Phabricator via cfe-commits]

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

Looks good for D/U, but looks like --help and --version options are also 
supported as combined short args; do you mind adding those too while you're 
here?

Thanks for the patch!




Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:68
+  -h --help - Display available options
+  --version - Display the version of this program
+  -D- Use zero for timestamps and uids/gids (default)

`-v --version`



Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1164-1176
+} else if (arg.consume_front("-")) {
+  // Handle the -D/-U flag
+  while (!arg.empty()) {
+if (arg.front() == 'D') {
+  Deterministic = true;
+} else if (arg.front() == 'U') {
+  Deterministic = false;

```
  } else if (arg.front() == 'h') {
printHelpMessage();
return 0;
  } else if (arg.front() == 'v') {
cl::PrintVersionMessage();
return 0;
  }
```

Actually, `ranlib -vh` and `ranlib -hv` (on my machine at least) both print the 
help message. I don't think we need *that* level of compatibility though -- we 
can go with first-one-wins.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71554



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