[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

[Iain Sandoe via Phabricator via cfe-commits]

iains added a comment.

@ChuanqiXu BTW, I think we (unfortunately) have some overlap in work in 
progress here;

I have implemented a similar scheme to GCC's where C/BMIs can be produced "on 
demand" when the FE encounters the module keywords.  The idea was to make it 
possible to have the same command lines on both compilers (basically to remove 
the need for the user to identify that a c++ source is is modular - with the 
sole exception of header units which the compiler cannot figure out on its own).

This scheme interfaces with the module-mapper approach (but how the mapping is 
determined could just as easily be from command line flags, module map files 
etc. etc. that is the beauty of that approach - the compiler has no need to 
know about how the mapping is made - it can be a fixed in-process scheme (like 
using the module cache path + some well-known mapping of names) or a full-blown 
build system - e.g. attaching to build2 (I think Boris did try out an earlier 
version of my implementation.)

draft patches (need rebasing) here: D118896 , 
D118898 ,D118895 
, D118899  
(these do not include the libCody stuff, which would support the module mapper, 
but concentrate on the compiler side).

Reworking/rebasing this scheme and posting the patches is next on my TODO after 
(current work on) P1815  part 2.

Hopefully, there is some way to combine these pieces of work to give the user 
greatest flexibility.


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

https://reviews.llvm.org/D134267

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

[clang-tools-extra] 1ae33bf - [clang-tidy] Add cppcoreguidelines-avoid-do-while check

[Carlos Galvez via cfe-commits]

Author: Carlos Galvez
Date: 2022-10-10T07:29:17Z
New Revision: 1ae33bf42680b156fe0f5cd6163bf24ef45d8cd3

URL: 
https://github.com/llvm/llvm-project/commit/1ae33bf42680b156fe0f5cd6163bf24ef45d8cd3
DIFF: 
https://github.com/llvm/llvm-project/commit/1ae33bf42680b156fe0f5cd6163bf24ef45d8cd3.diff

LOG: [clang-tidy] Add cppcoreguidelines-avoid-do-while check

Implements rule ES.75 of C++ Core Guidelines.

Differential Revision: https://reviews.llvm.org/D132461

Added: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidDoWhileCheck.cpp
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidDoWhileCheck.h

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-do-while.rst

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-do-while.cpp

Modified: 
clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt

clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst

Removed: 




diff  --git 
a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidDoWhileCheck.cpp 
b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidDoWhileCheck.cpp
new file mode 100644
index 0..1746b98714946
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidDoWhileCheck.cpp
@@ -0,0 +1,41 @@
+//===--- AvoidDoWhileCheck.cpp - clang-tidy 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "AvoidDoWhileCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+AvoidDoWhileCheck::AvoidDoWhileCheck(StringRef Name, ClangTidyContext *Context)
+: ClangTidyCheck(Name, Context),
+  IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", false)) {}
+
+void AvoidDoWhileCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IgnoreMacros", IgnoreMacros);
+}
+
+void AvoidDoWhileCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(doStmt().bind("x"), this);
+}
+
+void AvoidDoWhileCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *MatchedDecl = Result.Nodes.getNodeAs("x")) {
+if (IgnoreMacros && MatchedDecl->getBeginLoc().isMacroID())
+  return;
+diag(MatchedDecl->getBeginLoc(), "avoid do-while loops");
+  }
+}
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang

diff  --git 
a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidDoWhileCheck.h 
b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidDoWhileCheck.h
new file mode 100644
index 0..757842bf525bf
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidDoWhileCheck.h
@@ -0,0 +1,41 @@
+//===--- AvoidDoWhileCheck.h - clang-tidy ---*- C++ 
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDDOWHILECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDDOWHILECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+/// do-while loops are less readable than plan while loops, and can lead to
+/// subtle bugs.
+///
+/// For the user-facing documentation see:
+/// 
http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/avoid-do-while.html
+class AvoidDoWhileCheck : public ClangTidyCheck {
+public:
+  AvoidDoWhileCheck(StringRef Name, ClangTidyContext *Context);
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+return LangOpts.CPlusPlus;
+  }
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  bool IgnoreMacros;
+};
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+#endif // 
LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDDOWHILECHECK_H

diff  --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt 
b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
index 832b0bf9d9e5e..6fb11858e44a1 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
+++ b/clang-too

[PATCH] D132461: [clang-tidy] Add cppcoreguidelines-avoid-do-while check

[Carlos Galvez via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1ae33bf42680: [clang-tidy] Add 
cppcoreguidelines-avoid-do-while check (authored by carlosgalvezp).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132461

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidDoWhileCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidDoWhileCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-do-while.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-do-while.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-do-while.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-do-while.cpp
@@ -0,0 +1,88 @@
+// RUN: %check_clang_tidy -check-suffixes=DEFAULT   %s cppcoreguidelines-avoid-do-while %t
+// RUN: %check_clang_tidy -check-suffixes=IGNORE-MACROS %s cppcoreguidelines-avoid-do-while %t -- -config='{CheckOptions: [{key: cppcoreguidelines-avoid-do-while.IgnoreMacros, value: true}]}'
+
+#define FOO(x) \
+  do { \
+  } while (x != 0)
+
+#define BAR_0(x) \
+  do { \
+bar(x); \
+  } while (0)
+
+#define BAR_FALSE(x) \
+  do { \
+bar(x); \
+  } while (false)
+
+void bar(int);
+int baz();
+
+void foo()
+{
+// CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+2]]:5: warning: avoid do-while loops [cppcoreguidelines-avoid-do-while]
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops [cppcoreguidelines-avoid-do-while]
+do {
+
+} while(0);
+
+// CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+2]]:5: warning: avoid do-while loops
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+do {
+
+} while(1);
+
+// CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+2]]:5: warning: avoid do-while loops
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+do {
+
+} while(-1);
+
+// CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+2]]:5: warning: avoid do-while loops
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+do {
+
+} while(false);
+
+// CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+2]]:5: warning: avoid do-while loops
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+do {
+
+} while(true);
+
+// CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+3]]:5: warning: avoid do-while loops
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+2]]:5: warning: avoid do-while loops
+int x1{0};
+do {
+  x1 = baz();
+} while (x1 > 0);
+
+// CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+2]]:5: warning: avoid do-while loops
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+do {
+
+} while (x1 != 0);
+
+// CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+3]]:5: warning: avoid do-while loops
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+2]]:5: warning: avoid do-while loops
+constexpr int x2{0};
+do {
+
+} while (x2);
+
+// CHECK-MESSAGES-IGNORE-MACROS: :[[@LINE+3]]:5: warning: avoid do-while loops
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+2]]:5: warning: avoid do-while loops
+constexpr bool x3{false};
+do {
+
+} while (x3);
+
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+FOO(x1);
+
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+BAR_0(x1);
+
+// CHECK-MESSAGES-DEFAULT: :[[@LINE+1]]:5: warning: avoid do-while loops
+BAR_FALSE(x1);
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -178,6 +178,7 @@
`concurrency-mt-unsafe `_,
`concurrency-thread-canceltype-asynchronous `_,
`cppcoreguidelines-avoid-const-or-ref-data-members `_,
+   `cppcoreguidelines-avoid-do-while `_,
`cppcoreguidelines-avoid-goto `_,
`cppcoreguidelines-avoid-non-const-global-variables `_,
`cppcoreguidelines-init-variables `_, "Yes"
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-do-while.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-do-while.rst
@@ -0,0 +1,38 @@
+.. title:: clang-tidy - cppcoreguidelines-avoid-do-while
+
+cppcoreguidelines-avoid-do-while
+
+
+Warns when using ``do-while`` loops. They are less readable than plain ``while``
+loops, since the termination condition is at the end

[PATCH] D135433: [clang][Interp] Implement while and do-while loops

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.



Comment at: clang/test/AST/Interp/loops.cpp:9
+
+namespace WhileLoop {
+  constexpr int f() {

shafik wrote:
> infinite loop w/o a side effect are undefined behavior and so should be 
> ill-formed and generate a diagnostic e.g. `while(1);`, so we should check 
> these cases. 
I think that's better done with a more general approach that limits the 
iteration count for all loops like the current interpreter does.  But that 
would probably blow up this patch too much.

Unfortunately I can't add test case for the case you describe because the clang 
process with the new interpreter would never terminate :) Can add a 
commented-out version though.


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

https://reviews.llvm.org/D135433

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

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D134267#3846153 , @iains wrote:

> @ChuanqiXu BTW, I think we (unfortunately) have some overlap in work in 
> progress here;
>
> I have implemented a similar scheme to GCC's where C/BMIs can be produced "on 
> demand" when the FE encounters the module keywords.  The idea was to make it 
> possible to have the same command lines on both compilers (basically to 
> remove the need for the user to identify that a c++ source is is modular - 
> with the sole exception of header units which the compiler cannot figure out 
> on its own).
>
> This scheme interfaces with the module-mapper approach (but how the mapping 
> is determined could just as easily be from command line flags, module map 
> files etc. etc. that is the beauty of that approach - the compiler has no 
> need to know about how the mapping is made - it can be a fixed in-process 
> scheme (like using the module cache path + some well-known mapping of names) 
> or a full-blown build system - e.g. attaching to build2 (I think Boris did 
> try out an earlier version of my implementation.)
>
> draft patches (need rebasing) here: D118896 
> , D118898 
> ,D118895 
> , D118899 
>  (these do not include the libCody stuff, 
> which would support the module mapper, but concentrate on the compiler side).
> edit: D118895  is probably no longer 
> relevant.
>
> Reworking/rebasing this scheme and posting the patches is next on my TODO 
> after (current work on) P1815  part 2.
>
> Hopefully, there is some way to combine these pieces of work to give the user 
> greatest flexibility.

@iains oh, I remembered just now that you said you'd like to take this in one 
of GitHub issues long ago. My bad. I forgot it when I started this. I think you 
can add me as subscriber such drafts next time to avoid similar things happen 
again.
In fact, I had some drafts about modules and they conflicted with yours 
previously. So I'm sure I understand this : (

And for this concrete job (generate pcm and object file in one compilation 
job), I think my implementation looks better. It is shorter and reuses the 
current behavior (generate `.pcm` files in `/tmp` directories and we just 
redirect it). So I think it may be better to pursue this patch for this task. 
(The option design could be changed of course).

---

(following off may not be related to this patch)
And it is definitely valuable to have command line interfaces compatibility 
between clang and gcc. But for the libcody things, we must need a RFC in the 
discourse first. Also for the module mapper things, I'm not sure if we should 
pursue it. (I'm not against it. I'm just hesitating). For example, I am trying 
to implement the proposal P1689  from kitware 
guys in 
https://github.com/ChuanqiXu9/llvm-project/tree/MyP1689/P1689Examples/HelloWorld.
 And it looks good that we can compile a hello world example now. So I'm not 
sure if the gcc's module mapper is necessary here. Maybe we need more 
discussing.


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

https://reviews.llvm.org/D134267

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

[PATCH] D135508: [clangd] Heuristic to avoid desync if editors are confused about newline-at-eof

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

i also would rather not have the workaround solely for an editor (we usually 
try to address these on editor/plugin side). i am also worried about the 
understanding of that inferred line afterwards (e.g. what if editor thinks that 
line doesn't have a trailing `\n` and send edits with that view of the world, 
or somehow attributes the `\n` to the next line instead)

moreover, this won't fix the issue for existing clangd's (and until the next 
release). so i'd rather get it fixed on the client side (and if we can't, i 
guess editors that can't have delta changes reliably just send full text as 
changes).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135508

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

[PATCH] D135422: [clang-format] Fix misattributing preprocessor directives to macros

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:2986-2990
+  verifyFormat("#define X  
"
+   "\\\n"
+   "  case 0: break;\n"
+   "#include \"f\"\n",
+   Style);

We can move it down to take advantage of the shorter `ColumnLimit`.



Comment at: clang/unittests/Format/FormatTest.cpp:3006
Style);
   Style.ColumnLimit = 21;
   verifyFormat("switch (a) {\n"

We usually don't append a newline at the end of a test case.


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

https://reviews.llvm.org/D135422

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

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

[Iain Sandoe via Phabricator via cfe-commits]

iains added a comment.

In D134267#3846218 , @ChuanqiXu wrote:

> In D134267#3846153 , @iains wrote:
>
>> @ChuanqiXu BTW, I think we (unfortunately) have some overlap in work in 
>> progress here;
>>
>> I have implemented a similar scheme to GCC's where C/BMIs can be produced 
>> "on demand" when the FE encounters the module keywords.  The idea was to 
>> make it possible to have the same command lines on both compilers (basically 
>> to remove the need for the user to identify that a c++ source is is modular 
>> - with the sole exception of header units which the compiler cannot figure 
>> out on its own).
>>
>> This scheme interfaces with the module-mapper approach (but how the mapping 
>> is determined could just as easily be from command line flags, module map 
>> files etc. etc. that is the beauty of that approach - the compiler has no 
>> need to know about how the mapping is made - it can be a fixed in-process 
>> scheme (like using the module cache path + some well-known mapping of names) 
>> or a full-blown build system - e.g. attaching to build2 (I think Boris did 
>> try out an earlier version of my implementation.)
>>
>> draft patches (need rebasing) here: D118896 
>> , D118898 
>> ,D118895 
>> , D118899 
>>  (these do not include the libCody stuff, 
>> which would support the module mapper, but concentrate on the compiler side).
>> edit: D118895  is probably no longer 
>> relevant.
>>
>> Reworking/rebasing this scheme and posting the patches is next on my TODO 
>> after (current work on) P1815  part 2.
>>
>> Hopefully, there is some way to combine these pieces of work to give the 
>> user greatest flexibility.
>
> @iains oh, I remembered just now that you said you'd like to take this in one 
> of GitHub issues long ago. My bad. I forgot it when I started this. I think 
> you can add me as subscriber such drafts next time to avoid similar things 
> happen again.
> In fact, I had some drafts about modules and they conflicted with yours 
> previously. So I'm sure I understand this : (
>
> And for this concrete job (generate pcm and object file in one compilation 
> job), I think my implementation looks better. It is shorter and reuses the 
> current behavior (generate `.pcm` files in `/tmp` directories and we just 
> redirect it). So I think it may be better to pursue this patch for this task. 
> (The option design could be changed of course).

I need to review this patch some more - but at first looking it seems to be 
driver-only - so I am not sure how you are arranging for the PCM  **and** the 
object to  be created from a single invocation of the FE.  The idea of my patch 
series is to match the behavior o GCC - where the FE can figure out 
(independently of any file suffix) that it needs to emit an BMI.

> ---
>
> (following off may not be related to this patch)

perhaps - but a small response here (we can take this to a different forum if 
needed)

> And it is definitely valuable to have command line interfaces compatibility 
> between clang and gcc. But for the libcody things, we must need a RFC in the 
> discourse first.

Well the idea had also been discussed among the 'modules group' but. sure we 
can this - however the module-mapper is not required to make this initial patch 
series useful on its own - it is about automatically producing the BMI without 
special command lines.

> Also for the module mapper things, I'm not sure if we should pursue it. (I'm 
> not against it. I'm just hesitating). For example, I am trying to implement 
> the proposal P1689  from kitware guys in 
> https://github.com/ChuanqiXu9/llvm-project/tree/MyP1689/P1689Examples/HelloWorld.
>  And it looks good that we can compile a hello world example now. So I'm not 
> sure if the gcc's module mapper is necessary here. Maybe we need more 
> discussing.

There is almost certainly more than one solution to this and different 
solutions will fit different scenarios - Cmake and build2 are examples, but 
actually it's quite nice with a small project to be able to use an in-process 
resolver for names and not require any external build system.  I realise a lot 
of discussion in SG15 is targeted at the larger scale problems, but we should 
also provide (as you suggest) for "hello world" - preferably without the user 
having to install a separate package to make it work.


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

https://reviews.llvm.org/D134267

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

[PATCH] D134303: [AST] Preserve more structure in UsingEnumDecl node.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

Modelling this as a `TypeLoc` for code reuse reasons gives me a pause too.
However, I think we still accept this as keeping the clients simpler seems like 
the right trade-off to me.

Putting information about both typedefs and qualifiers explicitly will make the 
representation of `UsingEnumDecl` way too complicated for a seemingly simple 
and niche feature.




Comment at: clang-tools-extra/clangd/unittests/SelectionTests.cpp:554
+)cpp",
+   "UsingEnumDecl"},
   };

Could we also test that checks what happens in presence of typedefs?
```
namespace enums {
  enum class X { a, b};
  using Y = X;
}

namespace uses {
using enum enums::Y;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134303

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

[PATCH] D70401: [RISCV] Complete RV32E/ilp32e implementation

[Zixuan Wu via Phabricator via cfe-commits]

zixuan-wu added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:106
+for (size_t Reg = RISCV::X16; Reg <= RISCV::X31; Reg++)
+  markSuperRegs(Reserved, Reg);
+

I am wondering whether we need construct another new RegisterClass for RV32E 
instead of GPR, for example eGPR, so that the num and other info such as 
weight, etc of RegisterClass can adjust. Then the reserved logic is not 
necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70401

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

[PATCH] D134303: [AST] Preserve more structure in UsingEnumDecl node.

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.

it looks good to me, just a few optional nits.




Comment at: clang/include/clang/AST/DeclCXX.h:3618
+  /// The source location of the 'enum' keyword.
+  SourceLocation EnumLoc;
+  /// 'qual::SomeEnum' as an EnumType, possibly with Elaborated/Typedef sugar.

nit: we only rename  `EnumLocation` but not the `UsingLocation`, it is a little 
wired, I think either we rename both or keep both unchanged.




Comment at: clang/include/clang/AST/DeclCXX.h:3623
   /// The enum
   EnumDecl *Enum;
 

I think we can probably get rid of the `Enum` field, since we are storing the 
EnumType and we can get the EnumDecl by 
`dyn_cast(EnumType->getType()->getAsTagDecl())`



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11857
   assert(!SS->isInvalid() && "ScopeSpec is invalid");
-  ParsedType TypeRep = getTypeName(II, IdentLoc, S, SS);
-  if (!TypeRep) {
+  TypeSourceInfo *TSI;
+  QualType EnumTy = GetTypeFromParser(

nit: adding `= nullptr` initializer


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134303

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

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D134267#3846264 , @iains wrote:

> In D134267#3846218 , @ChuanqiXu 
> wrote:
>
>> In D134267#3846153 , @iains wrote:
>>
>>> @ChuanqiXu BTW, I think we (unfortunately) have some overlap in work in 
>>> progress here;
>>>
>>> I have implemented a similar scheme to GCC's where C/BMIs can be produced 
>>> "on demand" when the FE encounters the module keywords.  The idea was to 
>>> make it possible to have the same command lines on both compilers 
>>> (basically to remove the need for the user to identify that a c++ source is 
>>> is modular - with the sole exception of header units which the compiler 
>>> cannot figure out on its own).
>>>
>>> This scheme interfaces with the module-mapper approach (but how the mapping 
>>> is determined could just as easily be from command line flags, module map 
>>> files etc. etc. that is the beauty of that approach - the compiler has no 
>>> need to know about how the mapping is made - it can be a fixed in-process 
>>> scheme (like using the module cache path + some well-known mapping of 
>>> names) or a full-blown build system - e.g. attaching to build2 (I think 
>>> Boris did try out an earlier version of my implementation.)
>>>
>>> draft patches (need rebasing) here: D118896 
>>> , D118898 
>>> ,D118895 
>>> , D118899 
>>>  (these do not include the libCody stuff, 
>>> which would support the module mapper, but concentrate on the compiler 
>>> side).
>>> edit: D118895  is probably no longer 
>>> relevant.
>>>
>>> Reworking/rebasing this scheme and posting the patches is next on my TODO 
>>> after (current work on) P1815  part 2.
>>>
>>> Hopefully, there is some way to combine these pieces of work to give the 
>>> user greatest flexibility.
>>
>> @iains oh, I remembered just now that you said you'd like to take this in 
>> one of GitHub issues long ago. My bad. I forgot it when I started this. I 
>> think you can add me as subscriber such drafts next time to avoid similar 
>> things happen again.
>> In fact, I had some drafts about modules and they conflicted with yours 
>> previously. So I'm sure I understand this : (
>>
>> And for this concrete job (generate pcm and object file in one compilation 
>> job), I think my implementation looks better. It is shorter and reuses the 
>> current behavior (generate `.pcm` files in `/tmp` directories and we just 
>> redirect it). So I think it may be better to pursue this patch for this 
>> task. (The option design could be changed of course).
>
> I need to review this patch some more - but at first looking it seems to be 
> driver-only - so I am not sure how you are arranging for the PCM  **and** the 
> object to  be created from a single invocation of the FE.  The idea of my 
> patch series is to match the behavior o GCC - where the FE can figure out 
> (independently of any file suffix) that it needs to emit an BMI.

Let me add some more words to ease the understand. The secret here is that 
clang knows it is compiling a module unit before parsing - by the suffix 
`.cppm` or specified `-xc++-module` explicitly. And the clang would always 
generate the pcm file and the object file for the module unit in the past time 
too. The original behavior to compile a `*.cppm` to `*.o` would be:

1. Compile `*.cppm` to `/tmp/random-name.pcm`
2. Compile `/tmp/random-name.pcm` to `*.o`

And the only change this patch made is to change the destination of 
`/tmp/random-name.pcm`.

BTW, the meaning of `--precompile` (or `-emit-module-interface` in Xclang) is 
to stop at the precompilation step instead of doing precompilation. Since 
precompilation would always happen for module unit.

>> ---
>>
>> (following off may not be related to this patch)
>
> perhaps - but a small response here (we can take this to a different forum if 
> needed)
>
>> And it is definitely valuable to have command line interfaces compatibility 
>> between clang and gcc. But for the libcody things, we must need a RFC in the 
>> discourse first.
>
> Well the idea had also been discussed among the 'modules group' but. sure we 
> can this - however the module-mapper is not required to make this initial 
> patch series useful on its own - it is about automatically producing the BMI 
> without special command lines.
>
>> Also for the module mapper things, I'm not sure if we should pursue it. (I'm 
>> not against it. I'm just hesitating). For example, I am trying to implement 
>> the proposal P1689  from kitware guys in 
>> https://github.com/ChuanqiXu9/llvm-project/tree/MyP1689/P1689Examples/HelloWorld.
>>  And it looks good that we can compile a hello world example now. So I'm not 
>> sure if 

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

[Iain Sandoe via Phabricator via cfe-commits]

iains added a comment.

In D134267#3846264 , @iains wrote:

> In D134267#3846218 , @ChuanqiXu 
> wrote:
>
>> In D134267#3846153 , @iains wrote:
>>
>>> @ChuanqiXu BTW, I think we (unfortunately) have some overlap in work in 
>>> progress here;
>>>
>>> I have implemented a similar scheme to GCC's where C/BMIs can be produced 
>>> "on demand" when the FE encounters the module keywords.  The idea was to 
>>> make it possible to have the same command lines on both compilers 
>>> (basically to remove the need for the user to identify that a c++ source is 
>>> is modular - with the sole exception of header units which the compiler 
>>> cannot figure out on its own).
>>>
>>> This scheme interfaces with the module-mapper approach (but how the mapping 
>>> is determined could just as easily be from command line flags, module map 
>>> files etc. etc. that is the beauty of that approach - the compiler has no 
>>> need to know about how the mapping is made - it can be a fixed in-process 
>>> scheme (like using the module cache path + some well-known mapping of 
>>> names) or a full-blown build system - e.g. attaching to build2 (I think 
>>> Boris did try out an earlier version of my implementation.)
>>>
>>> draft patches (need rebasing) here: D118896 
>>> , D118898 
>>> ,D118895 
>>> , D118899 
>>>  (these do not include the libCody stuff, 
>>> which would support the module mapper, but concentrate on the compiler 
>>> side).
>>> edit: D118895  is probably no longer 
>>> relevant.



In D134267#3846334 , @ChuanqiXu wrote:

> In D134267#3846264 , @iains wrote:
>
>> In D134267#3846218 , @ChuanqiXu 
>> wrote:
>>
>>> In D134267#3846153 , @iains wrote:
>>>
 @ChuanqiXu BTW, I think we (unfortunately) have some overlap in work in 
 progress here;

 I have implemented a similar scheme to GCC's where C/BMIs can be produced 
 "on demand" when the FE encounters the module keywords.  The idea was to 
 make it possible to have the same command lines on both compilers 
 (basically to remove the need for the user to identify that a c++ source 
 is is modular - with the sole exception of header units which the compiler 
 cannot figure out on its own).

 This scheme interfaces with the module-mapper approach (but how the 
 mapping is determined could just as easily be from command line flags, 
 module map files etc. etc. that is the beauty of that approach - the 
 compiler has no need to know about how the mapping is made - it can be a 
 fixed in-process scheme (like using the module cache path + some 
 well-known mapping of names) or a full-blown build system - e.g. attaching 
 to build2 (I think Boris did try out an earlier version of my 
 implementation.)

 draft patches (need rebasing) here: D118896 
 , D118898 
 ,D118895 
 , D118899 
  (these do not include the libCody 
 stuff, which would support the module mapper, but concentrate on the 
 compiler side).
 edit: D118895  is probably no longer 
 relevant.

 Reworking/rebasing this scheme and posting the patches is next on my TODO 
 after (current work on) P1815  part 2.

 Hopefully, there is some way to combine these pieces of work to give the 
 user greatest flexibility.
>>>
>>> @iains oh, I remembered just now that you said you'd like to take this in 
>>> one of GitHub issues long ago. My bad. I forgot it when I started this. I 
>>> think you can add me as subscriber such drafts next time to avoid similar 
>>> things happen again.
>>> In fact, I had some drafts about modules and they conflicted with yours 
>>> previously. So I'm sure I understand this : (
>>>
>>> And for this concrete job (generate pcm and object file in one compilation 
>>> job), I think my implementation looks better. It is shorter and reuses the 
>>> current behavior (generate `.pcm` files in `/tmp` directories and we just 
>>> redirect it). So I think it may be better to pursue this patch for this 
>>> task. (The option design could be changed of course).
>>
>> I need to review this patch some more - but at first looking it seems to be 
>> driver-only - so I am not sure how you are arranging for the PCM  **and** 
>> the object to  be created from a single invocation of the FE.  The idea of 
>> my patch series is to match 

[PATCH] D134529: [C++20][Clang] P2468R2 The Equality Operator You Are Looking For

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D134529#3845617 , @sberg wrote:

> I just ran into newly-failing
> It looks to me like this is correctly rejected now per P2468R2.

Yes, this is intentional, the behavior of the code is the same as in C++17.

> But it is rather confusing that a note mentions the `S2` candidate, while the 
> relevant `S1` operators `==` and `!=` are not mentioned at all.

The diagnostic is indeed not very helpful and it would be nice to improve it.
However, this is unrelated to this change, e.g. it reproduces 
 in C++17 mode with Clang 10.

In D134529#3845050 , @royjacobson 
wrote:

> Should cxx_status.html be updated as well? It's listed there as a C++2b paper.

@usaxena95 we have forgotten to do this indeed. Could you please update the 
relevant html file?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134529

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

[PATCH] D135461: [LLDB] Fix crash when printing a struct with a static wchar_t member

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

You need to update the python file for the test as well.

What does the output look like, do we actually print the character itself or 
it's value? (just curious, which one we do is probably out of scope for this 
change)




Comment at: clang/lib/AST/StmtPrinter.cpp:1298
+  case BuiltinType::WChar_U:
+break; // no suffix.
   }

Is there any reason to have a suffix here?

I admit this function is puzzling to me anyway given that char has a prefix and 
this won't. Perhaps this is because char here is not "character" it's an 
integer of a certain size. Where wide char is more about, well, being an actual 
character.

And reading https://en.cppreference.com/w/cpp/language/character_literal the 
suffix would be "L" which we've already used.

As long as it prints in a way that's useful for the developer that's fine.



Comment at: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp:38
   std::numeric_limits::max();
+  const static auto wchar_max = std::numeric_limits::max();
 

Is there a specific `signed wchar_t` and `unsigned wchar_t` like for char?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135461

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

[PATCH] D135508: [clangd] Heuristic to avoid desync if editors are confused about newline-at-eof

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D135508#3846231 , @kadircet wrote:

> i also would rather not have the workaround solely for an editor (we usually 
> try to address these on editor/plugin side).

Yeah, for sure. My main concern is that in practice it won't get fixed, but 
I'll open a new bug and see (the old one is closed because we thought it was 
fixed).

> i am also worried about the understanding of that inferred line afterwards 
> (e.g. what if editor thinks that line doesn't have a trailing `\n` and send 
> edits with that view of the world, or somehow attributes the `\n` to the next 
> line instead)

The former can't really happen: the only edits that can touch this phantom 
newline either a) replace it, or b) replace text after it.
In the a) case it's gone so we'd agree with the editor, in the b) case... I 
can't imagine how a line-array-based editor could `x` on line 3 and then later 
claim line 2 and 3 are the same line as "I never added the newline".

> moreover, this won't fix the issue for existing clangd's (and until the next 
> release).

This is very true, though it cuts both ways: if we fix nvim then nvim 0.5-0.8 
remain broken. The release & update cadence there is pretty similar to clangd.

> so i'd rather get it fixed on the client side (and if we can't, i guess 
> editors that can't have delta changes reliably just send full text as 
> changes).

Turning off delta is an interesting idea, I'll mention that on the bug.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135508

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

[PATCH] D135067: [lit] RUN commands without stdin.

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: llvm/utils/lit/tests/Inputs/shtest-stdin/print-stdin.py:6
+print(sys.stdin.read())
\ No newline at end of file


Nit: add newline at EOF.



Comment at: llvm/utils/lit/tests/shtest-stdin.py:28
+# CHECK-PIPE: foobar
+

Nit: Don't have multiple \n at EOF (there should be exactly one).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135067

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

[PATCH] D127403: [clangd] Implement semantic token modifier "definition"

[Christian Kandeler via Phabricator via cfe-commits]

ckandeler added a comment.

So, is this okay to merge now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127403

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

[PATCH] D129689: [limits.h] USHRT_MAX fix for 16 bit architectures

[Sebastian Perta via Phabricator via cfe-commits]

SebastianPerta added a comment.

>>are you planning to update the patch
Yes, sorry about the delay. I will update the patch before the end of the week.


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

https://reviews.llvm.org/D129689

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

[PATCH] D135366: [clang][Interp] Implement String- and CharacterLiterals

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 466461.

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

https://reviews.llvm.org/D135366

Files:
  clang/lib/AST/Interp/ByteCodeExprGen.cpp
  clang/lib/AST/Interp/ByteCodeExprGen.h
  clang/test/AST/Interp/arrays.cpp


Index: clang/test/AST/Interp/arrays.cpp
===
--- clang/test/AST/Interp/arrays.cpp
+++ clang/test/AST/Interp/arrays.cpp
@@ -117,3 +117,27 @@
// expected-error {{must be initialized 
by a constant expression}} \
// expected-note {{cannot refer to 
element -2 of array of 10}}
 };
+
+namespace strings {
+  constexpr const char *S = "abc";
+  static_assert(S[0] == 'a', "");
+  static_assert(S[1] == 'b', "");
+  static_assert(S[2] == 'c', "");
+  static_assert(S[3] == '\0', "");
+
+  static_assert("foobar"[2] == 'o', "");
+  static_assert(2["foobar"] == 'o', "");
+
+  constexpr const wchar_t *wide = L"bar";
+  static_assert(wide[0] == L'b', "");
+
+  constexpr const char32_t *u32 = U"abc";
+  static_assert(u32[1] == U'b', "");
+
+  constexpr char32_t c = U'\U0001F60E';
+  static_assert(c == U'😎');
+
+  constexpr char k = -1;
+  static_assert(k == -1);
+
+};
Index: clang/lib/AST/Interp/ByteCodeExprGen.h
===
--- clang/lib/AST/Interp/ByteCodeExprGen.h
+++ clang/lib/AST/Interp/ByteCodeExprGen.h
@@ -90,6 +90,8 @@
   bool VisitArrayInitIndexExpr(const ArrayInitIndexExpr *E);
   bool VisitOpaqueValueExpr(const OpaqueValueExpr *E);
   bool VisitAbstractConditionalOperator(const AbstractConditionalOperator *E);
+  bool VisitStringLiteral(const StringLiteral *E);
+  bool VisitCharacterLiteral(const CharacterLiteral *E);
 
 protected:
   bool visitExpr(const Expr *E) override;
Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -433,6 +433,21 @@
   return true;
 }
 
+template 
+bool ByteCodeExprGen::VisitStringLiteral(const StringLiteral *E) {
+  unsigned StringIndex = P.createGlobalString(E);
+  return this->emitGetPtrGlobal(StringIndex, E);
+}
+
+template 
+bool ByteCodeExprGen::VisitCharacterLiteral(
+const CharacterLiteral *E) {
+  if (Optional T = classify(E->getType()))
+return this->emitConst(E, E->getValue());
+
+  return this->bail(E);
+}
+
 template  bool ByteCodeExprGen::discard(const Expr *E) 
{
   OptionScope Scope(this, /*NewDiscardResult=*/true);
   return this->Visit(E);


Index: clang/test/AST/Interp/arrays.cpp
===
--- clang/test/AST/Interp/arrays.cpp
+++ clang/test/AST/Interp/arrays.cpp
@@ -117,3 +117,27 @@
// expected-error {{must be initialized by a constant expression}} \
// expected-note {{cannot refer to element -2 of array of 10}}
 };
+
+namespace strings {
+  constexpr const char *S = "abc";
+  static_assert(S[0] == 'a', "");
+  static_assert(S[1] == 'b', "");
+  static_assert(S[2] == 'c', "");
+  static_assert(S[3] == '\0', "");
+
+  static_assert("foobar"[2] == 'o', "");
+  static_assert(2["foobar"] == 'o', "");
+
+  constexpr const wchar_t *wide = L"bar";
+  static_assert(wide[0] == L'b', "");
+
+  constexpr const char32_t *u32 = U"abc";
+  static_assert(u32[1] == U'b', "");
+
+  constexpr char32_t c = U'\U0001F60E';
+  static_assert(c == U'😎');
+
+  constexpr char k = -1;
+  static_assert(k == -1);
+
+};
Index: clang/lib/AST/Interp/ByteCodeExprGen.h
===
--- clang/lib/AST/Interp/ByteCodeExprGen.h
+++ clang/lib/AST/Interp/ByteCodeExprGen.h
@@ -90,6 +90,8 @@
   bool VisitArrayInitIndexExpr(const ArrayInitIndexExpr *E);
   bool VisitOpaqueValueExpr(const OpaqueValueExpr *E);
   bool VisitAbstractConditionalOperator(const AbstractConditionalOperator *E);
+  bool VisitStringLiteral(const StringLiteral *E);
+  bool VisitCharacterLiteral(const CharacterLiteral *E);
 
 protected:
   bool visitExpr(const Expr *E) override;
Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -433,6 +433,21 @@
   return true;
 }
 
+template 
+bool ByteCodeExprGen::VisitStringLiteral(const StringLiteral *E) {
+  unsigned StringIndex = P.createGlobalString(E);
+  return this->emitGetPtrGlobal(StringIndex, E);
+}
+
+template 
+bool ByteCodeExprGen::VisitCharacterLiteral(
+const CharacterLiteral *E) {
+  if (Optional T = classify(E->getType()))
+return this->emitConst(E, E->getValue());
+
+  return this->bail(E);
+}
+
 template  bool ByteCodeExprGen::discard(const Expr *E) {
   OptionScope Scope(this, /*NewDiscardResult=*/

[PATCH] D135569: [clang][Interp] Don't run functions immediately after compiling them

[Timm Bäder via Phabricator via cfe-commits]

tbaeder created this revision.
tbaeder added reviewers: aaron.ballman, erichkeane, tahonermann, shafik.
Herald added a project: All.
tbaeder requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

I'm not 100% sure why this was done in the first place, but it doesn't work 
when the function makes assumptions about the stack of the caller, e.g. in the 
RVO case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135569

Files:
  clang/lib/AST/Interp/Context.cpp


Index: clang/lib/AST/Interp/Context.cpp
===
--- clang/lib/AST/Interp/Context.cpp
+++ clang/lib/AST/Interp/Context.cpp
@@ -44,8 +44,7 @@
   if (!Func->isConstexpr())
 return false;
 
-  APValue Dummy;
-  return Run(Parent, Func, Dummy);
+  return true;
 }
 
 bool Context::evaluateAsRValue(State &Parent, const Expr *E, APValue &Result) {


Index: clang/lib/AST/Interp/Context.cpp
===
--- clang/lib/AST/Interp/Context.cpp
+++ clang/lib/AST/Interp/Context.cpp
@@ -44,8 +44,7 @@
   if (!Func->isConstexpr())
 return false;
 
-  APValue Dummy;
-  return Run(Parent, Func, Dummy);
+  return true;
 }
 
 bool Context::evaluateAsRValue(State &Parent, const Expr *E, APValue &Result) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135397: [clang][dataflow] Add support for a Top value in boolean formulas.

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.



Comment at: 
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:156
 
+  TopBoolValue &createTopBoolValue() {
+return takeOwnership(std::make_unique());

Please add a comment.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:291
 
+  BoolValue &makeTopBoolValue() const {
+return DACtx->createTopBoolValue();

Please add a comment.



Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:100
+public:
+  explicit TopBoolValue() : BoolValue(Kind::TopBool) {}
+

`explicit` seems unnecessary.



Comment at: clang/lib/Analysis/FlowSensitive/WatchedLiteralsSolver.cpp:237-238
+  case Value::Kind::TopBool:
+// Nothing more to do. Each `TopBool` instance is mapped to a fresh
+// variable.
+break;

Where? Does that mean that `TopBool` never reaches the solver? I think it'd be 
good to clarify.



Comment at: 
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp:1220
+  (void)0;
+  /*[[p2]]*/
+}

Why do we need to check two code points here and in the test below? It's not 
obvious what the difference between `p1` and `p2` is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135397

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

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

[YingChi Long via Phabricator via cfe-commits]

inclyc updated this revision to Diff 466471.
inclyc added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fix CI issue


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135551

Files:
  clang/include/clang/AST/Redeclarable.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/Interp/ByteCodeExprGen.cpp
  clang/lib/Analysis/CFG.cpp
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Basic/Targets/NVPTX.cpp
  clang/lib/CodeGen/CGHLSLRuntime.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/Multilib.cpp
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Frontend/Rewrite/RewriteModernObjC.cpp
  clang/lib/Frontend/Rewrite/RewriteObjC.cpp
  clang/lib/Frontend/SARIFDiagnostic.cpp
  clang/lib/Lex/PPDirectives.cpp
  clang/lib/Lex/PreprocessingRecord.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
  clang/lib/StaticAnalyzer/Core/SVals.cpp
  clang/tools/clang-refactor/TestSupport.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXCursor.cpp
  clang/utils/TableGen/ClangSyntaxEmitter.cpp

Index: clang/utils/TableGen/ClangSyntaxEmitter.cpp
===
--- clang/utils/TableGen/ClangSyntaxEmitter.cpp
+++ clang/utils/TableGen/ClangSyntaxEmitter.cpp
@@ -117,7 +117,7 @@
 } else if (R.isSubClassOf("NodeType")) {
   NodeType = R.getName().str();
 } else {
-  assert(false && "Unhandled Syntax kind");
+  llvm_unreachable("Unhandled Syntax kind");
 }
   }
 
Index: clang/tools/libclang/CXCursor.cpp
===
--- clang/tools/libclang/CXCursor.cpp
+++ clang/tools/libclang/CXCursor.cpp
@@ -1487,12 +1487,12 @@
   TemplateArgument TA;
   if (clang_Cursor_getTemplateArgument(C, I, &TA) !=
   CXGetTemplateArgumentStatus_Success) {
-assert(0 && "Unable to retrieve TemplateArgument");
+llvm_unreachable("Unable to retrieve TemplateArgument");
 return 0;
   }
 
   if (TA.getKind() != TemplateArgument::Integral) {
-assert(0 && "Passed template argument is not Integral");
+llvm_unreachable("Passed template argument is not Integral");
 return 0;
   }
 
@@ -1504,12 +1504,12 @@
   TemplateArgument TA;
   if (clang_Cursor_getTemplateArgument(C, I, &TA) !=
   CXGetTemplateArgumentStatus_Success) {
-assert(0 && "Unable to retrieve TemplateArgument");
+llvm_unreachable("Unable to retrieve TemplateArgument");
 return 0;
   }
 
   if (TA.getKind() != TemplateArgument::Integral) {
-assert(0 && "Passed template argument is not Integral");
+llvm_unreachable("Passed template argument is not Integral");
 return 0;
   }
 
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -199,7 +199,7 @@
   if (clang_isDeclaration(Cursor.kind)) {
 const Decl *D = getCursorDecl(Cursor);
 if (!D) {
-  assert(0 && "Invalid declaration cursor");
+  llvm_unreachable("Invalid declaration cursor");
   return true; // abort.
 }
 
@@ -5188,7 +5188,7 @@
 return P->FullyQualifiedName;
   }
 
-  assert(false && "Invalid CXPrintingPolicyProperty");
+  llvm_unreachable("Invalid CXPrintingPolicyProperty");
   return 0;
 }
 
@@ -5280,7 +5280,7 @@
 return;
   }
 
-  assert(false && "Invalid CXPrintingPolicyProperty");
+  llvm_unreachable("Invalid CXPrintingPolicyProperty");
 }
 
 CXString clang_getCursorPrettyPrinted(CXCursor C, CXPrintingPolicy cxPolicy) {
Index: clang/tools/clang-refactor/TestSupport.cpp
===
--- clang/tools/clang-refactor/TestSupport.cpp
+++ clang/tools/clang-refactor/TestSupport.cpp
@@ -348,7 +348,7 @@
 if (!Matches[2].empty()) {
   // Don't forget to drop the '+'!
   if (Matches[2].drop_front().getAsInteger(10, ColumnOffset))
-assert(false && "regex should have produced a number");
+llvm_unreachable("regex should have produced a number");
 }
 Offset = addColumnOffset(Source, Offset, ColumnOffset);
 unsigned EndOffset;
@@ -365,7 +365,7 @@
   unsigned EndLineOffset = 0, EndColumn = 0;
   if (EndLocMatches[1].drop_front().getAsInteger(10, EndLineOffset) ||
   EndLocMatches[2].getAsInteger(10, EndColumn))
-assert(false && "regex should have produced a number");
+llvm_unreachable("regex should have produced a number");
   EndOffset = addEndLineOffsetAndEndColumn(Source, Offset, EndLineOffset,
End

[clang-tools-extra] 71b4bc1 - Fix clang-tools-extra Sphinx build

[Aaron Ballman via cfe-commits]

Author: Aaron Ballman
Date: 2022-10-10T07:55:26-04:00
New Revision: 71b4bc185f610ddac7c6334513392ff01820fd90

URL: 
https://github.com/llvm/llvm-project/commit/71b4bc185f610ddac7c6334513392ff01820fd90
DIFF: 
https://github.com/llvm/llvm-project/commit/71b4bc185f610ddac7c6334513392ff01820fd90.diff

LOG: Fix clang-tools-extra Sphinx build

This should address the issues found in:
https://lab.llvm.org/buildbot/#/builders/115/builds/35504

Added: 


Modified: 
clang-tools-extra/docs/ReleaseNotes.rst

Removed: 




diff  --git a/clang-tools-extra/docs/ReleaseNotes.rst 
b/clang-tools-extra/docs/ReleaseNotes.rst
index 7fe3d89afdefb..735fedbce5496 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -169,7 +169,7 @@ Changes in existing checks
   :doc:`readability-simplify-boolean-expr 
`
   when using a C++23 ``if consteval`` statement.
 
-- Improved :doc:`misc-redundant-expression 
`
+- Improved :doc:`misc-redundant-expression 
`
   check.
 
   The check now skips concept definitions since redundant expressions still 
make sense



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

[PATCH] D135097: Remove redundant option '-menable-unsafe-fp-math'.

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam updated this revision to Diff 466474.

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

https://reviews.llvm.org/D135097

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/fp-options-to-fast-math-flags.c
  clang/test/CodeGen/func-attr.c
  clang/test/CodeGen/libcalls.c
  clang/test/CodeGenCUDA/propagate-metadata.cu
  clang/test/Driver/cl-options.c
  clang/test/Driver/fast-math.c
  clang/test/Driver/fp-model.c
  clang/test/Parser/fp-floatcontrol-syntax.cpp
  clang/unittests/Frontend/CompilerInvocationTest.cpp

Index: clang/unittests/Frontend/CompilerInvocationTest.cpp
===
--- clang/unittests/Frontend/CompilerInvocationTest.cpp
+++ clang/unittests/Frontend/CompilerInvocationTest.cpp
@@ -705,7 +705,7 @@
 //
 //   * -cl-unsafe-math-optimizations
 // * -cl-mad-enable
-// * -menable-unsafe-fp-math
+// * -funsafe-math-optimizations
 //   * -freciprocal-math
 
 TEST_F(CommandLineTest, ImpliedBoolOptionsNoFlagPresent) {
@@ -723,7 +723,8 @@
   ASSERT_THAT(GeneratedArgs,
   Not(Contains(StrEq("-cl-unsafe-math-optimizations";
   ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-cl-mad-enable";
-  ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-menable-unsafe-fp-math";
+  ASSERT_THAT(GeneratedArgs,
+  Not(Contains(StrEq("-funsafe-math-optimizations";
   ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-freciprocal-math";
 }
 
@@ -745,13 +746,14 @@
   ASSERT_THAT(GeneratedArgs, Contains(StrEq("-cl-unsafe-math-optimizations")));
   // Not generated - implied by the generated root flag.
   ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-cl-mad-enable";
-  ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-menable-unsafe-fp-math";
+  ASSERT_THAT(GeneratedArgs,
+  Not(Contains(StrEq("-funsafe-math-optimizations";
   ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-freciprocal-math";
 }
 
 TEST_F(CommandLineTest, ImpliedBoolOptionsAllFlagsPresent) {
   const char *Args[] = {"-cl-unsafe-math-optimizations", "-cl-mad-enable",
-"-menable-unsafe-fp-math", "-freciprocal-math"};
+"-funsafe-math-optimizations", "-freciprocal-math"};
 
   ASSERT_TRUE(CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags));
   ASSERT_TRUE(Invocation.getLangOpts()->CLUnsafeMath);
@@ -765,12 +767,13 @@
   ASSERT_THAT(GeneratedArgs, Contains(StrEq("-cl-unsafe-math-optimizations")));
   // Not generated - implied by their generated parent.
   ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-cl-mad-enable";
-  ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-menable-unsafe-fp-math";
+  ASSERT_THAT(GeneratedArgs,
+  Not(Contains(StrEq("-funsafe-math-optimizations";
   ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-freciprocal-math";
 }
 
 TEST_F(CommandLineTest, ImpliedBoolOptionsImpliedFlagsPresent) {
-  const char *Args[] = {"-cl-mad-enable", "-menable-unsafe-fp-math",
+  const char *Args[] = {"-cl-mad-enable", "-funsafe-math-optimizations",
 "-freciprocal-math"};
 
   ASSERT_TRUE(CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags));
@@ -785,13 +788,13 @@
   Not(Contains(StrEq("-cl-unsafe-math-optimizations";
   // Generated - explicitly provided.
   ASSERT_THAT(GeneratedArgs, Contains(StrEq("-cl-mad-enable")));
-  ASSERT_THAT(GeneratedArgs, Contains(StrEq("-menable-unsafe-fp-math")));
+  ASSERT_THAT(GeneratedArgs, Contains(StrEq("-funsafe-math-optimizations")));
   // Not generated - implied by its generated parent.
   ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-freciprocal-math";
 }
 
 TEST_F(CommandLineTest, PresentAndNotImpliedGenerated) {
-  const char *Args[] = {"-cl-mad-enable", "-menable-unsafe-fp-math"};
+  const char *Args[] = {"-cl-mad-enable", "-funsafe-math-optimizations"};
 
   ASSERT_TRUE(CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags));
 
@@ -799,7 +802,7 @@
 
   // Present options that were not implied are generated.
   ASSERT_THAT(GeneratedArgs, Contains(StrEq("-cl-mad-enable")));
-  ASSERT_THAT(GeneratedArgs, Contains(StrEq("-menable-unsafe-fp-math")));
+  ASSERT_THAT(GeneratedArgs, Contains(StrEq("-funsafe-math-optimizations")));
 }
 
 // Diagnostic option.
Index: clang/test/Parser/fp-floatcontrol-syntax.cpp
===
--- clang/test/Parser/fp-floatcontrol-syntax.cpp
+++ clang/test/Parser/fp-floatcontrol-syntax.cpp
@@ -42,7 +42,7 @@
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -fdenormal-fp-math=preserve-sign,preserve-sign -fsyntax-only %s -DDEFAULT -verify
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only %s -ffp-contract=fast -DPRECISE -verify
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only %s -ffp-contract=off -frounding-mat

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3844788 , @BertalanD wrote:

> Hi @erichkeane,
>
> This change broke compilation of this program 
> (https://godbolt.org/z/KrWGvcf8h; reduced from 
> https://github.com/SerenityOS/ladybird):
>
>   template
>   constexpr bool IsSame = false;
>   
>   template
>   constexpr bool IsSame = true;
>   
>   template
>   struct Foo {
>   template
>   Foo(U&&) requires (!IsSame);
>   };
>   
>   template<>
>   struct Foo : Foo {
>   using Foo::Foo;
>   };
>   
>   Foo test() { return 0; }
>
>
>
>   :18:27: error: invalid reference to function 'Foo': constraints not 
> satisfied
>   Foo test() { return 0; }
> ^
>   :10:24: note: because substituted constraint expression is 
> ill-formed: value of type '' is not contextually convertible 
> to 'bool'
>   Foo(U&&) requires (!IsSame);
>  ^

Thanks for the report!  I'll look into it ASAP.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I don't know if that discussion reached a conclusion to move forward with this 
change -- my reading of the conversation was that efforts would be better spend 
on fuzzing instead of changing policy about using unreachable vs assert(0).

In general, I'm a bit hesitant to make this change. On the one hand, it's 
logically no worse than using `assert(0)` in a release build (if you hit this 
code path, bad things are going to happen). But `__builtin_unreachable` can 
have time travel optimization effects that `assert` doesn't have, and so the 
kind of bad things which can happen are different between the two (and use of 
unreachable on reachable code paths might make for harder debugging in 
RelWithDebInfo builds). Historically, we've usually used `llvm_unreachable` for 
situations where we're saying "this code cannot be reached; if it can, 
something else has gone seriously wrong." For example, in code like: `int 
foo(SomeEnum E) { switch (E) { case One: return 1; default: return 2; } 
llvm_unreachable("getting here would be mysterious"); }` and we've used 
`assert(0)` for situations where we're saying "this code is possible to reach 
only when there were mistakes elsewhere which broke invariants we were relying 
on." The two situations are similar, but still different enough that I don't 
think we should wholesale change from one form to another.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135551

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

[PATCH] D135341: [clang] adds `__reference_constructs_from_temporary`

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:397
+def err_reserved_identifier_for_future_use : Error<
+  "%0 is a compiler-reserved identifier for a future feature">;
 }

I don't think we should diagnose this for individual identifiers, I don't think 
this adds much value, and the identifier is already reserved for implementation 
use.  This implies we are going to diagnose ALL future reserved things, which 
we have no intention of doing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135341

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

[PATCH] D135177: [clang] adds `__is_scoped_enum`, `__is_nullptr`, and `__is_referenceable`

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/test/SemaCXX/type-traits.cpp:822
+
+  { int a[F(__is_referenceable(void))]; }
+}

aaron.ballman wrote:
> I think we should have test cases for the following:
> ```
> struct incomplete;
> 
> __is_referenceable(struct incomplete); // Also interesting to make sure we 
> handle elaborated type specifiers properly
> 
> typedef void function_type(int);
> __is_referenceable(function_type); // Note, this is not a function *pointer* 
> type
> 
> struct S {
>   void func1() &;
>   void func2() const;
> };
> 
> // Both of these should be false, by my understanding of what "referenceable" 
> means in the standard.
> __is_referenceable(decltype(&S::func1));
> __is_referenceable(decltype(&S::func2));
> ```
Do you have a test for:
```
struct S {
  void func1() &;
  void func2() const;
};

// Both of these should be false, by my understanding of what "referenceable" 
means in the standard.
__is_referenceable(decltype(&S::func1));
__is_referenceable(decltype(&S::func2));
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135177

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

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

[YingChi Long via Phabricator via cfe-commits]

inclyc added a comment.

In D135551#3846592 , @aaron.ballman 
wrote:

> I don't know if that discussion reached a conclusion to move forward with 
> this change -- my reading of the conversation was that efforts would be 
> better spend on fuzzing instead of changing policy about using unreachable vs 
> assert(0).
>
> In general, I'm a bit hesitant to make this change. On the one hand, it's 
> logically no worse than using `assert(0)` in a release build (if you hit this 
> code path, bad things are going to happen). But `__builtin_unreachable` can 
> have time travel optimization effects that `assert` doesn't have, and so the 
> kind of bad things which can happen are different between the two (and use of 
> unreachable on reachable code paths might make for harder debugging in 
> RelWithDebInfo builds). Historically, we've usually used `llvm_unreachable` 
> for situations where we're saying "this code cannot be reached; if it can, 
> something else has gone seriously wrong." For example, in code like: `int 
> foo(SomeEnum E) { switch (E) { case One: return 1; default: return 2; } 
> llvm_unreachable("getting here would be mysterious"); }` and we've used 
> `assert(0)` for situations where we're saying "this code is possible to reach 
> only when there were mistakes elsewhere which broke invariants we were 
> relying on." The two situations are similar, but still different enough that 
> I don't think we should wholesale change from one form to another.



> but still different enough that I don't think we should wholesale change from 
> one form to another.

In general we can control the behavior here via `-DLLVM_UNREACHABLE_OPTIMIZE` 
to choose making assumptions or traps (looks better than assertions to me).

https://github.com/llvm/llvm-project/blob/50312ea133999cb2aad1ab9ef0ec39429a9427c5/llvm/include/llvm/Support/ErrorHandling.h#L125

(This change was landed 7 months ago https://reviews.llvm.org/D121750)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135551

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

[PATCH] D135366: [clang][Interp] Implement String- and CharacterLiterals

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a subscriber: cor3ntin.
aaron.ballman added inline comments.



Comment at: clang/test/AST/Interp/arrays.cpp:135
+  static_assert(u32[1] == U'b', "");
+};
+

shafik wrote:
> tbaeder wrote:
> > aaron.ballman wrote:
> > > I think you need a more coverage for character literals. Some test cases 
> > > that are interesting: multichar literals (`'abcd'`), character literals 
> > > with UCNs (`'\u'`), character literals with numeric escapes 
> > > (`'\xFF'`). I'm especially interested in seeing whether we handle integer 
> > > promotions properly, especially when promoting to `unsigned int`.
> > I added two more test cases but I'm generally not that familiar with 
> > character  literal edge cases and integer promotion, so if you have 
> > concrete test cases in mind, that would be great :)
> We can find GNU documentation of multi-char literals here: 
> https://gcc.gnu.org/onlinedocs/cpp/Implementation-defined-behavior.html#Implementation-defined-behavior
>  and I believe we follow the same scheme. 
> 
> There are some weird cases like `'\8'` which compilers seem to treat 
> consistently but generate a diagnostic for.
CC @tahonermann and @cor3ntin as text encoding code owners -- they likely know 
all the worst test cases to be thinking about in this space.


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

https://reviews.llvm.org/D135366

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

[clang] 6685e56 - Disallow dereferencing of void* in C++.

[Erich Keane via cfe-commits]

Author: Erich Keane
Date: 2022-10-10T07:11:46-07:00
New Revision: 6685e56ceddf7c88eb3c39e838a8164941aade05

URL: 
https://github.com/llvm/llvm-project/commit/6685e56ceddf7c88eb3c39e838a8164941aade05
DIFF: 
https://github.com/llvm/llvm-project/commit/6685e56ceddf7c88eb3c39e838a8164941aade05.diff

LOG: Disallow dereferencing of void* in C++.

as Discussed:
https://discourse.llvm.org/t/rfc-can-we-stop-the-extension-to-allow-dereferencing-void-in-c/65708

There is no good reason to allow this when the other compilers all
reject this, and it messes with SFINAE/constraint checking.

Differential Revision: https://reviews.llvm.org/D135287

Added: 
clang/test/SemaCXX/disallow_void_deref.cpp

Modified: 
clang/docs/ReleaseNotes.rst
clang/include/clang/Basic/DiagnosticGroups.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaExpr.cpp
clang/test/CXX/temp/temp.decls/temp.class/temp.mem.func/p1inst.cpp
clang/test/SemaCXX/reinterpret-cast.cpp

Removed: 




diff  --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 513a5eda80801..16e6522a44273 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -117,6 +117,18 @@ code bases.
 
   These errors also match MSVC's behavior.
 
+- Clang now diagnoses indirection of ``void *`` in C++ mode as a warning which
+  defaults to an error. This is compatible with ISO C++, GCC, ICC, and MSVC. 
This
+  is also now a SFINAE error so constraint checking and SFINAE checking can be
+  compatible with other compilers. It is expected that this will be upgraded to
+  an error-only diagnostic in the next Clang release.
+
+  .. code-block:: c++
+
+  void func(void *p) {
+*p; // Now diagnosed as a warning-as-error.
+  }
+
 What's New in Clang |release|?
 ==
 Some of the major new features and improvements to Clang are listed

diff  --git a/clang/include/clang/Basic/DiagnosticGroups.td 
b/clang/include/clang/Basic/DiagnosticGroups.td
index ff88c8acec4c1..cddb127cae58f 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -961,6 +961,7 @@ def PointerToEnumCast : DiagGroup<"pointer-to-enum-cast",
   [VoidPointerToEnumCast]>;
 def PointerToIntCast : DiagGroup<"pointer-to-int-cast",
  [PointerToEnumCast, VoidPointerToIntCast]>;
+def VoidPointerDeref : DiagGroup<"void-ptr-dereference">;
 
 def FUseLdPath : DiagGroup<"fuse-ld-path">;
 

diff  --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 8a721d45e78f0..d6fbaed126d64 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6936,8 +6936,11 @@ def err_typecheck_unary_expr : Error<
 def err_typecheck_indirection_requires_pointer : Error<
   "indirection requires pointer operand (%0 invalid)">;
 def ext_typecheck_indirection_through_void_pointer : ExtWarn<
-  "ISO %select{C|C++}0 does not allow indirection on operand of type %1">,
-  InGroup>;
+  "ISO C does not allow indirection on operand of type %0">,
+  InGroup;
+def ext_typecheck_indirection_through_void_pointer_cpp
+: ExtWarn<"ISO C++ does not allow indirection on operand of type %0">,
+  InGroup, DefaultError, SFINAEFailure;
 def warn_indirection_through_null : Warning<
   "indirection of non-volatile null pointer will be deleted, not trap">,
   InGroup;

diff  --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 6d28a44952318..474f86cffd169 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -14536,9 +14536,12 @@ static QualType CheckIndirectionOperand(Sema &S, Expr 
*Op, ExprValueKind &VK,
 //   [...] the expression to which [the unary * operator] is applied shall
 //   be a pointer to an object type, or a pointer to a function type
 LangOptions LO = S.getLangOpts();
-if (LO.CPlusPlus || (!(LO.C99 && IsAfterAmp) && !S.isUnevaluatedContext()))
+if (LO.CPlusPlus)
+  S.Diag(OpLoc, diag::ext_typecheck_indirection_through_void_pointer_cpp)
+  << OpTy << Op->getSourceRange();
+else if (!(LO.C99 && IsAfterAmp) && !S.isUnevaluatedContext())
   S.Diag(OpLoc, diag::ext_typecheck_indirection_through_void_pointer)
-  << LO.CPlusPlus << OpTy << Op->getSourceRange();
+  << OpTy << Op->getSourceRange();
   }
 
   // Dereferences are usually l-values...

diff  --git 
a/clang/test/CXX/temp/temp.decls/temp.class/temp.mem.func/p1inst.cpp 
b/clang/test/CXX/temp/temp.decls/temp.class/temp.mem.func/p1inst.cpp
index eb11e3375e5f6..6d591457ae149 100644
--- a/clang/test/CXX/temp/temp.decls/temp.class/temp.mem.func/p1inst.cpp
+++ b/clang/test/CXX/temp/temp.decls/temp.class/temp.mem.func/p1inst.cpp
@@ -8,7 +8,7 @@ struct X0 {
 
 template
 void X0::f(T *t, const U

[PATCH] D135287: Disallow dereferencing of void* in C++.

[Erich Keane 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 rG6685e56ceddf: Disallow dereferencing of void* in C++. 
(authored by erichkeane).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D135287?vs=465725&id=466493#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135287

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/CXX/temp/temp.decls/temp.class/temp.mem.func/p1inst.cpp
  clang/test/SemaCXX/disallow_void_deref.cpp
  clang/test/SemaCXX/reinterpret-cast.cpp

Index: clang/test/SemaCXX/reinterpret-cast.cpp
===
--- clang/test/SemaCXX/reinterpret-cast.cpp
+++ clang/test/SemaCXX/reinterpret-cast.cpp
@@ -214,11 +214,11 @@
   (void)*reinterpret_cast(v_ptr);
 
   // Casting to void pointer
-  (void)*reinterpret_cast(&a); // expected-warning {{ISO C++ does not allow}}
-  (void)*reinterpret_cast(&b); // expected-warning {{ISO C++ does not allow}}
-  (void)*reinterpret_cast(&l); // expected-warning {{ISO C++ does not allow}}
-  (void)*reinterpret_cast(&d); // expected-warning {{ISO C++ does not allow}}
-  (void)*reinterpret_cast(&f); // expected-warning {{ISO C++ does not allow}}
+  (void)*reinterpret_cast(&a); // expected-error {{ISO C++ does not allow}}
+  (void)*reinterpret_cast(&b); // expected-error {{ISO C++ does not allow}}
+  (void)*reinterpret_cast(&l); // expected-error {{ISO C++ does not allow}}
+  (void)*reinterpret_cast(&d); // expected-error {{ISO C++ does not allow}}
+  (void)*reinterpret_cast(&f); // expected-error {{ISO C++ does not allow}}
 }
 
 void reinterpret_cast_allowlist () {
Index: clang/test/SemaCXX/disallow_void_deref.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/disallow_void_deref.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -fsyntax-only -verify=enabled,sfinae -std=c++20 %s
+// RUN: %clang_cc1 -fsyntax-only -verify=sfinae -std=c++20 -Wno-void-ptr-dereference %s
+
+void f(void* p) {
+  (void)*p; // enabled-error{{ISO C++ does not allow indirection on operand of type 'void *'}}
+}
+
+template
+concept deref = requires (T& t) {
+  { *t }; // #FAILED_REQ
+};
+
+static_assert(deref);
+// sfinae-error@-1{{static assertion failed}}
+// sfinae-note@-2{{because 'void *' does not satisfy 'deref'}}
+// sfinae-note@#FAILED_REQ{{because '*t' would be invalid: ISO C++ does not allow indirection on operand of type 'void *'}}
Index: clang/test/CXX/temp/temp.decls/temp.class/temp.mem.func/p1inst.cpp
===
--- clang/test/CXX/temp/temp.decls/temp.class/temp.mem.func/p1inst.cpp
+++ clang/test/CXX/temp/temp.decls/temp.class/temp.mem.func/p1inst.cpp
@@ -8,7 +8,7 @@
 
 template
 void X0::f(T *t, const U &u) {
-  *t = u; // expected-warning{{indirection on operand of type 'void *'}} expected-error{{not assignable}}
+  *t = u; // expected-error{{indirection on operand of type 'void *'}} expected-error{{not assignable}}
 }
 
 void test_f(X0 xfi, X0 xvi, float *fp, void *vp, int i) {
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -14536,9 +14536,12 @@
 //   [...] the expression to which [the unary * operator] is applied shall
 //   be a pointer to an object type, or a pointer to a function type
 LangOptions LO = S.getLangOpts();
-if (LO.CPlusPlus || (!(LO.C99 && IsAfterAmp) && !S.isUnevaluatedContext()))
+if (LO.CPlusPlus)
+  S.Diag(OpLoc, diag::ext_typecheck_indirection_through_void_pointer_cpp)
+  << OpTy << Op->getSourceRange();
+else if (!(LO.C99 && IsAfterAmp) && !S.isUnevaluatedContext())
   S.Diag(OpLoc, diag::ext_typecheck_indirection_through_void_pointer)
-  << LO.CPlusPlus << OpTy << Op->getSourceRange();
+  << OpTy << Op->getSourceRange();
   }
 
   // Dereferences are usually l-values...
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6936,8 +6936,11 @@
 def err_typecheck_indirection_requires_pointer : Error<
   "indirection requires pointer operand (%0 invalid)">;
 def ext_typecheck_indirection_through_void_pointer : ExtWarn<
-  "ISO %select{C|C++}0 does not allow indirection on operand of type %1">,
-  InGroup>;
+  "ISO C does not allow indirection on operand of type %0">,
+  InGroup;
+def ext_typecheck_indirection_through_void_pointer_cpp
+: ExtWarn<"ISO C++ does not allow indirection on operand of type %0">,
+ 

[PATCH] D135500: [Clang] reject bit-fields as instruction operands in Microsoft style inline asm blocks.

[Erich Keane via Phabricator via cfe-commits]

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

1 nit, otherwise LGTM.




Comment at: clang/lib/Sema/SemaStmtAsm.cpp:937
   for (uint64_t I = 0; I < NumOutputs + NumInputs; ++I) {
-if (Exprs[I]->getType()->isBitIntType())
-  return StmtError(
-  Diag(Exprs[I]->getBeginLoc(), diag::err_asm_invalid_type)
+if (Exprs[I]->getType()->isBitIntType()) {
+  InvalidOperand = true;

There is enough repetition of Exprs[I] I suspect there is value to splitting 
them up as:

`Expr *CurExpr = Exprs[I];`

for readability purposes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135500

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

[PATCH] D131858: [clang] Track the templated entity in type substitution.

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.

This LGTM, but please give @davrec time (a few days?) to do another review 
before merging.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

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

[PATCH] D127403: [clangd] Implement semantic token modifier "definition"

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

Thanks! Sorry for letting this drop.




Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:79
+  if (auto *Var = dyn_cast(Decl))
+return !isa(Var) && Var->isThisDeclarationADefinition();
+  return isa(Decl) || isa(Decl);

ckandeler wrote:
> I'm not 100% sure about this one, by the way. I've just never heard anyone 
> talk about a "parameter definition".
ParmVarDecls should be marked as definition. They definitely are in the formal 
C++ sense, and they're so similar to local variables that I think we have to be 
consistent.
(There are other formally-definitions that we're not marking here like 
NamespaceDecl, but they seem unlikely to cause confusion).

> I've just never heard anyone talk about a "parameter definition".

Yeah, I suspect this is mostly because:
 - there's no definition/declaration distinction - you can't forward-declare a 
param.
 - imprecise mental models of params as aliases to the written args rather than 
copies of them (at least I sometimes think this way)

But I think it's important we treat these consistently with e.g. local 
variables since they're so similar.





Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:62
+  if (T.Modifiers & (1 << I)) {
+if (I != uint32_t(HighlightingModifier::Declaration) || !hasDef)
+  OS << '_' << static_cast(I);

can you add a brief comment here: "// _decl_def is common and redundant, just 
print _def instead."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127403

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

[clang] 4baa5ae - Possibly fix sphinx regression from 6685e56ceddf

[Erich Keane via cfe-commits]

Author: Erich Keane
Date: 2022-10-10T07:22:41-07:00
New Revision: 4baa5aec79aadce13c7b876eb487be13a3c88d48

URL: 
https://github.com/llvm/llvm-project/commit/4baa5aec79aadce13c7b876eb487be13a3c88d48
DIFF: 
https://github.com/llvm/llvm-project/commit/4baa5aec79aadce13c7b876eb487be13a3c88d48.diff

LOG: Possibly fix sphinx regression from 6685e56ceddf

It isn't clear that this is what it is complaining about, but looking
closer, I think this is what I got wrong here. So submitting this to
hope that the Sphinx build will be happy.

Added: 


Modified: 
clang/docs/ReleaseNotes.rst

Removed: 




diff  --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 16e6522a4427..54f1afd06b28 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -125,9 +125,9 @@ code bases.
 
   .. code-block:: c++
 
-  void func(void *p) {
-*p; // Now diagnosed as a warning-as-error.
-  }
+void func(void *p) {
+  *p; // Now diagnosed as a warning-as-error.
+}
 
 What's New in Clang |release|?
 ==



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

[PATCH] D135555: [libclang] CIndex: Visit UsingTypeLoc as TypeRef

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/unittests/libclang/TestUtils.h:85
   }
+  static std::string from_CXString(CXString cx_string) {
+std::string string{clang_getCString(cx_string)};

nit: if you're moving this function here, I think you should also rename it to 
either fromCXString (matches style guide) or FromCXString (matches surrounding 
code)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D13

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

[PATCH] D132131: [clang-format] Adds a formatter for aligning trailing comments over empty lines

[Yusuke Kadowaki via Phabricator via cfe-commits]

yusuke-kadowaki updated this revision to Diff 466504.
yusuke-kadowaki added a comment.

Implment trailing comments Leave option

There is two options, I think, when leaving comments exceeds the column limit.

1. Just format the trailing comments
2. Ignore the column limit for that line.

This implementation does the first option because ignoring column limit semms 
too much for just taking care of trailing comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132131

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestComments.cpp

Index: clang/unittests/Format/FormatTestComments.cpp
===
--- clang/unittests/Format/FormatTestComments.cpp
+++ clang/unittests/Format/FormatTestComments.cpp
@@ -2858,6 +2858,213 @@
"int a; //\n");
 }
 
+TEST_F(FormatTestComments, AlignTrailingCommentsAcrossEmptyLines) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AlignTrailingComments.Kind = FormatStyle::TCAS_Always;
+  Style.AlignTrailingComments.OverEmptyLines = 1;
+  verifyFormat("#include \"a.h\"  // simple\n"
+   "\n"
+   "#include \"aa.h\" // example case\n",
+   Style);
+
+  verifyFormat("#include \"a.h\"   // align across\n"
+   "\n"
+   "#include \"aa.h\"  // two empty lines\n"
+   "\n"
+   "#include \"aaa.h\" // in a row\n",
+   Style);
+
+  verifyFormat("#include \"a.h\"  // align\n"
+   "#include \"aa.h\" // comment\n"
+   "#include \"aaa.h\"// blocks\n"
+   "\n"
+   "#include \".h\"   // across\n"
+   "#include \"a.h\"  // one\n"
+   "#include \"aa.h\" // empty line\n",
+   Style);
+
+  verifyFormat("#include \"a.h\"  // align trailing comments\n"
+   "#include \"a.h\"\n"
+   "#include \"aa.h\" // across a line without comment\n",
+   Style);
+
+  verifyFormat("#include \"a.h\"   // align across\n"
+   "#include \"a.h\"\n"
+   "#include \"aa.h\"  // two lines without comment\n"
+   "#include \"a.h\"\n"
+   "#include \"aaa.h\" // in a row\n",
+   Style);
+
+  verifyFormat("#include \"a.h\"  // align\n"
+   "#include \"aa.h\" // comment\n"
+   "#include \"aaa.h\"// blocks\n"
+   "#include \"a.h\"\n"
+   "#include \".h\"   // across\n"
+   "#include \"a.h\"  // a line without\n"
+   "#include \"aa.h\" // comment\n",
+   Style);
+
+  // Start of testing OverEmptyLines
+  Style.MaxEmptyLinesToKeep = 3;
+  Style.AlignTrailingComments.OverEmptyLines = 2;
+  // Cannot use verifyFormat here
+  // test::messUp removes all new lines which changes the logic
+  EXPECT_EQ("#include \"a.h\" // comment\n"
+"\n"
+"\n"
+"\n"
+"#include \"ab.h\"  // comment\n"
+"\n"
+"\n"
+"#include \"abcdefg.h\" // comment\n",
+format("#include \"a.h\" // comment\n"
+   "\n"
+   "\n"
+   "\n"
+   "#include \"ab.h\" // comment\n"
+   "\n"
+   "\n"
+   "#include \"abcdefg.h\" // comment\n",
+   Style));
+
+  Style.MaxEmptyLinesToKeep = 1;
+  Style.AlignTrailingComments.OverEmptyLines = 1;
+  // End of testing OverEmptyLines
+
+  Style.ColumnLimit = 15;
+  EXPECT_EQ("int ab; // line\n"
+"int a;  // long\n"
+"// long\n"
+"\n"
+"// long",
+format("int ab; // line\n"
+   "int a; // long long\n"
+   "\n"
+   "// long",
+   Style));
+
+  Style.ColumnLimit = 15;
+  EXPECT_EQ("int ab; // line\n"
+"\n"
+"int a;  // long\n"
+"// long\n",
+format("int ab; // line\n"
+   "\n"
+   "int a; // long long\n",
+   Style));
+
+  Style.ColumnLimit = 30;
+  EXPECT_EQ("int foo = 12345; // comment\n"
+"int bar =\n"
+"1234;  // This is a very\n"
+"   // long comment\n"
+"   // which is wrapped\n"
+"   // arround.\n"
+"\n"
+"int x = 2; // Is this still\n"
+"   // aligned?\n",
+format("int foo = 12345; // comment\n"
+   "int bar = 1234; // This is a very long comment\n"
+   "// w

[PATCH] D132131: [clang-format] Adds a formatter for aligning trailing comments over empty lines

[Yusuke Kadowaki via Phabricator via cfe-commits]

yusuke-kadowaki marked an inline comment as done.
yusuke-kadowaki added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:865
+
+  * ``TCAS_DontAlign`` (in configuration: ``DontAlign``)
+Don't align trailing comments.

HazardyKnusperkeks wrote:
> yusuke-kadowaki wrote:
> > HazardyKnusperkeks wrote:
> > > yusuke-kadowaki wrote:
> > > > HazardyKnusperkeks wrote:
> > > > > MyDeveloperDay wrote:
> > > > > > yusuke-kadowaki wrote:
> > > > > > > MyDeveloperDay wrote:
> > > > > > > > Is Don'tAlign the same as Leave that other options support  
> > > > > > > > (for options it best if we try and use the same terminiology
> > > > > > > > 
> > > > > > > > Always,Never,Leave (if that fits)
> > > > > > > IMHO, `Leave` probably fits but `DontAlign`  is more 
> > > > > > > straightforward. Also `DontAlign` is used for other alignment 
> > > > > > > styles like `BracketAlignmentStyle` so it would be more natural.
> > > > > > Leave should mean do nothing.. I'm personally not a fan of 
> > > > > > DontAlign because obvious there should be a ' between the n and the 
> > > > > > t but I see we use it elsewhere
> > > > > > 
> > > > > > But actually now I see your DontAlign is effectively the as before 
> > > > > > (i.e. it will not add extra spaces) 
> > > > > > 
> > > > > > The point of Leave is what people have been asking for when we 
> > > > > > introduce a new option, that is we if possible add an option which 
> > > > > > means "Don't touch it at all"  i.e.  if I want to have
> > > > > > 
> > > > > > ```
> > > > > > int a;   //  abc
> > > > > > int b;   /// bcd
> > > > > > int c;// cde
> > > > > > ```
> > > > > > 
> > > > > > Then so be it
> > > > > > 
> > > > > > 
> > > > > Leave is a nice option, yes.
> > > > > I think it is complementary to `Dont`.
> > > > > 
> > > > > But maybe if the option is called `AlignTrailingComments` one may 
> > > > > interpret `Leave` not as "don't touch the position of a comment at 
> > > > > all" (what do we do, if the comment is outside of the column limit?), 
> > > > > but as "just don't touch them, when they are somewhat aligned". Just 
> > > > > a thought.
> > > > > Leave should mean do nothing
> > > > 
> > > > Ok now I see what `Leave` means. 
> > > > But that should be another piece of work no? 
> > > > 
> > > > Of course me or someone can add the feature later (I don't really know 
> > > > how to implement that though at least for now) 
> > > > 
> > > > 
> > > Fine by me.
> > @HazardyKnusperkeks 
> > Is this complicated? If it's not I might as well do this.
> > 
> > Also it would be helpful if you could provide some implementation guidance. 
> > Sorry to ask this even though I haven't tried it myself yet.
> > @HazardyKnusperkeks 
> > Is this complicated? If it's not I might as well do this.
> > 
> > Also it would be helpful if you could provide some implementation guidance. 
> > Sorry to ask this even though I haven't tried it myself yet.
> 
> I think you refer to the non aligning of comments following r_braces. I don't 
> think so, because at least the cases I think about the r_brace should be the 
> first token on a unwrapped line, so you just have to check for Line->First.
Isn't this for https://github.com/llvm/llvm-project/issues/57504?
I thought that's different than leaving all the trailing comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132131

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

[PATCH] D135366: [clang][Interp] Implement String- and CharacterLiterals

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/test/AST/Interp/arrays.cpp:135
+  static_assert(u32[1] == U'b', "");
+};
+

aaron.ballman wrote:
> shafik wrote:
> > tbaeder wrote:
> > > aaron.ballman wrote:
> > > > I think you need a more coverage for character literals. Some test 
> > > > cases that are interesting: multichar literals (`'abcd'`), character 
> > > > literals with UCNs (`'\u'`), character literals with numeric 
> > > > escapes (`'\xFF'`). I'm especially interested in seeing whether we 
> > > > handle integer promotions properly, especially when promoting to 
> > > > `unsigned int`.
> > > I added two more test cases but I'm generally not that familiar with 
> > > character  literal edge cases and integer promotion, so if you have 
> > > concrete test cases in mind, that would be great :)
> > We can find GNU documentation of multi-char literals here: 
> > https://gcc.gnu.org/onlinedocs/cpp/Implementation-defined-behavior.html#Implementation-defined-behavior
> >  and I believe we follow the same scheme. 
> > 
> > There are some weird cases like `'\8'` which compilers seem to treat 
> > consistently but generate a diagnostic for.
> CC @tahonermann and @cor3ntin as text encoding code owners -- they likely 
> know all the worst test cases to be thinking about in this space.
Most weirdness are taken care of during lexing.
What is interesting to test during evaluation

 * multi character literals ie `'abcd'`
 * `u8` (utf8) and `u` literals (utf16)

Note that wide characters literals were removed from C++ so it's no longer a 
concern


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

https://reviews.llvm.org/D135366

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

[PATCH] D127403: [clangd] Implement semantic token modifier "definition"

[Christian Kandeler via Phabricator via cfe-commits]

ckandeler updated this revision to Diff 466510.
ckandeler marked 2 inline comments as done.
ckandeler added a comment.

Use modifier also for parameters,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127403

Files:
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/test/initialize-params.test
  clang-tools-extra/clangd/test/semantic-tokens.test
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/AnnotateHighlightingsTests.cpp

Index: clang-tools-extra/clangd/unittests/tweaks/AnnotateHighlightingsTests.cpp
===
--- clang-tools-extra/clangd/unittests/tweaks/AnnotateHighlightingsTests.cpp
+++ clang-tools-extra/clangd/unittests/tweaks/AnnotateHighlightingsTests.cpp
@@ -18,18 +18,18 @@
 TEST_F(AnnotateHighlightingsTest, Test) {
   EXPECT_AVAILABLE("^vo^id^ ^f(^) {^}^"); // available everywhere.
   EXPECT_AVAILABLE("[[int a; int b;]]");
-  EXPECT_EQ("void /* Function [decl] [globalScope] */f() {}",
+  EXPECT_EQ("void /* Function [decl] [def] [globalScope] */f() {}",
 apply("void ^f() {}"));
 
   EXPECT_EQ(apply("[[int f1(); const int x = f1();]]"),
 "int /* Function [decl] [globalScope] */f1(); "
-"const int /* Variable [decl] [readonly] [fileScope] */x = "
+"const int /* Variable [decl] [def] [readonly] [fileScope] */x = "
 "/* Function [globalScope] */f1();");
 
   // Only the targeted range is annotated.
   EXPECT_EQ(apply("void f1(); void f2() {^}"),
 "void f1(); "
-"void /* Function [decl] [globalScope] */f2() {}");
+"void /* Function [decl] [def] [globalScope] */f2() {}");
 }
 
 } // namespace
Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -48,12 +48,21 @@
 assert(StartOffset <= EndOffset);
 assert(NextChar <= StartOffset);
 
+bool hasDef =
+T.Modifiers & (1 << uint32_t(HighlightingModifier::Definition));
+bool hasDecl =
+T.Modifiers & (1 << uint32_t(HighlightingModifier::Declaration));
+EXPECT_TRUE(!hasDef || hasDecl);
+
 OS << Input.substr(NextChar, StartOffset - NextChar);
 OS << '$' << T.Kind;
 for (unsigned I = 0;
  I <= static_cast(HighlightingModifier::LastModifier); ++I) {
-  if (T.Modifiers & (1 << I))
-OS << '_' << static_cast(I);
+  if (T.Modifiers & (1 << I)) {
+// _decl_def is common and redundant, just print _def instead.
+if (I != uint32_t(HighlightingModifier::Declaration) || !hasDef)
+  OS << '_' << static_cast(I);
+  }
 }
 OS << "[[" << Input.substr(StartOffset, EndOffset - StartOffset) << "]]";
 NextChar = EndOffset;
@@ -96,52 +105,52 @@
 TEST(SemanticHighlighting, GetsCorrectTokens) {
   const char *TestCases[] = {
   R"cpp(
-  struct $Class_decl[[AS]] {
+  struct $Class_def[[AS]] {
 double $Field_decl[[SomeMember]];
   };
   struct {
-  } $Variable_decl[[S]];
-  void $Function_decl[[foo]](int $Parameter_decl[[A]], $Class[[AS]] $Parameter_decl[[As]]) {
-$Primitive_deduced_defaultLibrary[[auto]] $LocalVariable_decl[[VeryLongVariableName]] = 12312;
-$Class[[AS]] $LocalVariable_decl[[AA]];
-$Primitive_deduced_defaultLibrary[[auto]] $LocalVariable_decl[[L]] = $LocalVariable[[AA]].$Field[[SomeMember]] + $Parameter[[A]];
-auto $LocalVariable_decl[[FN]] = [ $LocalVariable[[AA]]](int $Parameter_decl[[A]]) -> void {};
+  } $Variable_def[[S]];
+  void $Function_def[[foo]](int $Parameter_def[[A]], $Class[[AS]] $Parameter_def[[As]]) {
+$Primitive_deduced_defaultLibrary[[auto]] $LocalVariable_def[[VeryLongVariableName]] = 12312;
+$Class[[AS]] $LocalVariable_def[[AA]];
+$Primitive_deduced_defaultLibrary[[auto]] $LocalVariable_def[[L]] = $LocalVariable[[AA]].$Field[[SomeMember]] + $Parameter[[A]];
+auto $LocalVariable_def[[FN]] = [ $LocalVariable[[AA]]](int $Parameter_def[[A]]) -> void {};
 $LocalVariable[[FN]](12312);
   }
 )cpp",
   R"cpp(
   void $Function_decl[[foo]](int);
   void $Function_decl[[Gah]]();
-  void $Function_decl[[foo]]() {
-auto $LocalVariable_decl[[Bou]] = $Function[[Gah]];
+  void $Function_def[[foo]]() {
+auto $LocalVariable_def[[Bou]] = $Function[[Gah]];
   }
-  struct $Class_decl[[A]] {
+  struct $Class_def[[A]] {
 void $Method_decl[[abc]]();
   };
 )cpp",
   R"cpp(
   namespace $Namespace_decl[[abc]] {
 template
-struct $Class_decl[[A]] {
+

[PATCH] D133066: fix a typo in comment of AddConversionCandidate

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D133066#3845136 , @zhouyizhou 
wrote:

> After 4 weeks' study, I think the comment didn't need to be changed, sorry to 
> have bring your so much trouble.

No worries at all, there was no trouble here!

> During this valuable process of studying, I grow up a lot. I learned to read 
> the C++ standard, and compare the standard to its implementation.
> In my case, the "user-defined conversion" is the variable "Candidate", the 
> "second standard conversion sequence" is the object member  
> "Candidate.FinalConversion".
> The only pity during my study is that I can't find a example code to let 
> Clang (even with commit cba72b1f620fd) hit the code below above comment.

I'm glad you found it valuable! As for a code example to hit the code below 
that comment, the closest I could come is:

  struct S {
operator int&& () const { return 12; }
  };
  
  void func(int &&i);
  
  int main() {
S s;
func(s);
  }

however, that still fails the lvalue-to-rvalue test. I poked at it for a while 
and I'm not seeing a case where it's possible to hit that condition (it doesn't 
mean one doesn't exist, just that I didn't have the time to chase it down 
fully).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133066

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

[PATCH] D135500: [Clang] reject bit-fields as instruction operands in Microsoft style inline asm blocks.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM as well with a diagnostic wording suggestion; please also add a release 
note for the fix.




Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:283
+  def err_ms_asm_bitfield_unsupported : Error<
+"bit-fields are not supported as operands in inline asm blocks">;
+

(Not strongly tied to the new wording, but we try to write diagnostics in 
singular form when possible and that led me to this rewording.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135500

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

[clang] b62f5fb - [Clang][Sema] Narrow inline namespace filtration for unqualified friend declarations

[Troy Johnson via cfe-commits]

Author: Troy Johnson
Date: 2022-10-10T08:34:14-07:00
New Revision: b62f5fb64edec7f5917a233bf0a9a30e1e4b70f0

URL: 
https://github.com/llvm/llvm-project/commit/b62f5fb64edec7f5917a233bf0a9a30e1e4b70f0
DIFF: 
https://github.com/llvm/llvm-project/commit/b62f5fb64edec7f5917a233bf0a9a30e1e4b70f0.diff

LOG: [Clang][Sema] Narrow inline namespace filtration for unqualified friend 
declarations

rG04ba1856 introduced a call to FilterLookupForScope that is expensive
for very large translation units where it was observed to cause a 6%
compile time degradation. As the comment states, the only effect is to
"remove declarations found in inline namespaces for friend declarations
with unqualified names." This change limits the call to that scenario.
The test that was added by rG04ba1856 continues to pass, but the
observed degradation is cut in half.

Differential Revision: https://reviews.llvm.org/D135370

Added: 


Modified: 
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

Removed: 




diff  --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp 
b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index b50ed6478e4b..dfae70209210 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -2273,9 +2273,11 @@ Decl *TemplateDeclInstantiator::VisitFunctionDecl(
 // Filter out previous declarations that don't match the scope. The only
 // effect this has is to remove declarations found in inline namespaces
 // for friend declarations with unqualified names.
-SemaRef.FilterLookupForScope(Previous, DC, /*Scope*/ nullptr,
- /*ConsiderLinkage*/ true,
- QualifierLoc.hasQualifier());
+if (isFriend && !QualifierLoc && !FunctionTemplate) {
+  SemaRef.FilterLookupForScope(Previous, DC, /*Scope=*/ nullptr,
+   /*ConsiderLinkage=*/ true,
+   QualifierLoc.hasQualifier());
+}
   }
 
   // Per [temp.inst], default arguments in function declarations at local scope



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

[PATCH] D135370: Narrow inline namespace filtration for unqualified friend declarations

[Troy Johnson 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 rGb62f5fb64ede: [Clang][Sema] Narrow inline namespace 
filtration for unqualified friend… (authored by troyj).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135370

Files:
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp


Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -2273,9 +2273,11 @@
 // Filter out previous declarations that don't match the scope. The only
 // effect this has is to remove declarations found in inline namespaces
 // for friend declarations with unqualified names.
-SemaRef.FilterLookupForScope(Previous, DC, /*Scope*/ nullptr,
- /*ConsiderLinkage*/ true,
- QualifierLoc.hasQualifier());
+if (isFriend && !QualifierLoc && !FunctionTemplate) {
+  SemaRef.FilterLookupForScope(Previous, DC, /*Scope=*/ nullptr,
+   /*ConsiderLinkage=*/ true,
+   QualifierLoc.hasQualifier());
+}
   }
 
   // Per [temp.inst], default arguments in function declarations at local scope


Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -2273,9 +2273,11 @@
 // Filter out previous declarations that don't match the scope. The only
 // effect this has is to remove declarations found in inline namespaces
 // for friend declarations with unqualified names.
-SemaRef.FilterLookupForScope(Previous, DC, /*Scope*/ nullptr,
- /*ConsiderLinkage*/ true,
- QualifierLoc.hasQualifier());
+if (isFriend && !QualifierLoc && !FunctionTemplate) {
+  SemaRef.FilterLookupForScope(Previous, DC, /*Scope=*/ nullptr,
+   /*ConsiderLinkage=*/ true,
+   QualifierLoc.hasQualifier());
+}
   }
 
   // Per [temp.inst], default arguments in function declarations at local scope
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135595: [HLSL] Add utility to convert environment to stage

[Chris Bieneman via Phabricator via cfe-commits]

beanz created this revision.
beanz added reviewers: aaron.ballman, MaskRay, klimek, python3kgae, pow2clk.
Herald added subscribers: Anastasia, StephenFan.
Herald added a project: All.
beanz requested review of this revision.
Herald added a project: clang.

We had a bunch of places in the code where we were translating triple
environment enum cases to shader stage enum cases. The order of these
enums needs to be kept in sync for the translation to be simple, but we
were not properly handling out-of-bounds cases.

In normal compilation out-of-bounds cases shouldn't be possible because
the driver errors if you don't have a valid shader environment set, but
in clang tooling that error doesn't get treated as fatal and parsing
continues. This can result in crashes in clang tooling for out-of-range
shader stages.

To address this, this patch adds a constexpr method to handle the
conversion which handles out-of-range values by converting them to
`Invalid`.

Since this new method is a constexpr, the tests for this are a group of
static_asserts in the implementation file that verifies the correct
conversion for each valid enum case and validates that other cases are
converted to `Invalid`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135595

Files:
  clang/include/clang/Basic/HLSLRuntime.h
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp

Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -24,6 +24,7 @@
 #include "clang/AST/Type.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/DarwinSDKInfo.h"
+#include "clang/Basic/HLSLRuntime.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
@@ -6819,12 +6820,12 @@
 static void handleHLSLNumThreadsAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   using llvm::Triple;
   Triple Target = S.Context.getTargetInfo().getTriple();
+  auto Env = S.Context.getTargetInfo().getTriple().getEnvironment();
   if (!llvm::is_contained({Triple::Compute, Triple::Mesh, Triple::Amplification,
Triple::Library},
-  Target.getEnvironment())) {
+  Env)) {
 uint32_t Pipeline =
-(uint32_t)S.Context.getTargetInfo().getTriple().getEnvironment() -
-(uint32_t)llvm::Triple::Pixel;
+static_cast(hlsl::getStageFromEnvironment(Env));
 S.Diag(AL.getLoc(), diag::err_hlsl_attr_unsupported_in_stage)
 << AL << Pipeline << "Compute, Amplification, Mesh or Library";
 return;
@@ -6891,16 +6892,13 @@
 
 static void handleHLSLSVGroupIndexAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   using llvm::Triple;
-  Triple Target = S.Context.getTargetInfo().getTriple();
-  if (Target.getEnvironment() != Triple::Compute &&
-  Target.getEnvironment() != Triple::Library) {
+  auto Env = S.Context.getTargetInfo().getTriple().getEnvironment();
+  if (Env != Triple::Compute && Env != Triple::Library) {
 // FIXME: it is OK for a compute shader entry and pixel shader entry live in
 // same HLSL file. Issue https://github.com/llvm/llvm-project/issues/57880.
-uint32_t Pipeline =
-(uint32_t)S.Context.getTargetInfo().getTriple().getEnvironment() -
-(uint32_t)llvm::Triple::Pixel;
+ShaderStage Pipeline = hlsl::getStageFromEnvironment(Env);
 S.Diag(AL.getLoc(), diag::err_hlsl_attr_unsupported_in_stage)
-<< AL << Pipeline << "Compute";
+<< AL << (uint32_t)Pipeline << "Compute";
 return;
   }
 
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -27,6 +27,7 @@
 #include "clang/AST/Randstruct.h"
 #include "clang/AST/StmtCXX.h"
 #include "clang/Basic/Builtins.h"
+#include "clang/Basic/HLSLRuntime.h"
 #include "clang/Basic/PartialDiagnostic.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetInfo.h"
@@ -10071,10 +10072,11 @@
 S->getDepth() == 0) {
   CheckHLSLEntryPoint(NewFD);
   if (!NewFD->isInvalidDecl()) {
-auto TripleShaderType = TargetInfo.getTriple().getEnvironment();
+auto Env = TargetInfo.getTriple().getEnvironment();
 AttributeCommonInfo AL(NewFD->getBeginLoc());
-HLSLShaderAttr::ShaderType ShaderType = (HLSLShaderAttr::ShaderType)(
-TripleShaderType - (uint32_t)llvm::Triple::Pixel);
+HLSLShaderAttr::ShaderType ShaderType =
+static_cast(
+hlsl::getStageFromEnvironment(Env));
 // To share code with HLSLShaderAttr, add HLSLShaderAttr to entry
 // function.
 if (HLSLShaderAttr *Attr = mergeHLSLShaderAttr(NewFD, AL, ShaderType))
Index: clang/lib/Frontend/InitPreprocessor.cpp
=

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D135551#3846603 , @inclyc wrote:

> In D135551#3846592 , @aaron.ballman 
> wrote:
>
>> I don't know if that discussion reached a conclusion to move forward with 
>> this change -- my reading of the conversation was that efforts would be 
>> better spend on fuzzing instead of changing policy about using unreachable 
>> vs assert(0).
>>
>> In general, I'm a bit hesitant to make this change. On the one hand, it's 
>> logically no worse than using `assert(0)` in a release build (if you hit 
>> this code path, bad things are going to happen). But `__builtin_unreachable` 
>> can have time travel optimization effects that `assert` doesn't have, and so 
>> the kind of bad things which can happen are different between the two (and 
>> use of unreachable on reachable code paths might make for harder debugging 
>> in RelWithDebInfo builds). Historically, we've usually used 
>> `llvm_unreachable` for situations where we're saying "this code cannot be 
>> reached; if it can, something else has gone seriously wrong." For example, 
>> in code like: `int foo(SomeEnum E) { switch (E) { case One: return 1; 
>> default: return 2; } llvm_unreachable("getting here would be mysterious"); 
>> }` and we've used `assert(0)` for situations where we're saying "this code 
>> is possible to reach only when there were mistakes elsewhere which broke 
>> invariants we were relying on." The two situations are similar, but still 
>> different enough that I don't think we should wholesale change from one form 
>> to another.
>
>
>
>> but still different enough that I don't think we should wholesale change 
>> from one form to another.
>
> In general we can control the behavior here via `-DLLVM_UNREACHABLE_OPTIMIZE` 
> to choose making assumptions or traps (looks better than assertions to me).
>
> https://github.com/llvm/llvm-project/blob/50312ea133999cb2aad1ab9ef0ec39429a9427c5/llvm/include/llvm/Support/ErrorHandling.h#L125
>
> (This change was landed 7 months ago https://reviews.llvm.org/D121750)

That doesn't change the underlying concern that `assert(0)` and `unreachable` 
are used for different purposes and trying to unify those use cases might lose 
some expressivity in the code base.

I've left some comments in the review about examples of my concerns (it's not 
an exhaustive review).




Comment at: clang/tools/libclang/CIndex.cpp:5191
 
-  assert(false && "Invalid CXPrintingPolicyProperty");
+  llvm_unreachable("Invalid CXPrintingPolicyProperty");
   return 0;

This one is a bit questionable -- this is part of the C interface we expose, 
which is ABI stable, so the assert was alerting users to potential mismatches 
between versions of the library.



Comment at: clang/tools/libclang/CXCursor.cpp:1490
   CXGetTemplateArgumentStatus_Success) {
-assert(0 && "Unable to retrieve TemplateArgument");
+llvm_unreachable("Unable to retrieve TemplateArgument");
 return 0;

Each of these is actually reachable -- the asserts exist specifically to tell 
users of the C interface about problems with their assumptions. In each of 
these cases, the assert is avoiding the need for a local variable to assert on.



Comment at: clang/utils/TableGen/ClangSyntaxEmitter.cpp:120
 } else {
-  assert(false && "Unhandled Syntax kind");
+  llvm_unreachable("Unhandled Syntax kind");
 }

This should not be using unreachable -- the code is very much reachable. This 
should have changed from `assert` to `PrintFatalError`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135551

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

[PATCH] D131963: [libc++] Add custom clang-tidy checks

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D131963#3845092 , @philnik wrote:

> In D131963#3831786 , @smeenai wrote:
>
>> In D131963#3811811 , @ldionne 
>> wrote:
>>
>>> I'd be fine with this as-is if it works in the CI.
>>>
>>> IIUC, the current blocker for this is that the `ClangConfig.cmake` 
>>> installed by LLVM is not robust to the dev packages missing. If you do 
>>> `find_package(Clang 16.0)`, it will error out if the dev packages are not 
>>> present, since it will try to reference static libraries (and other 
>>> artifacts) that don't exist and try to create IMPORTED targets for those. 
>>> @smeenai @beanz do you know more about that, or do you know someone that 
>>> does? Do you know who set up the CMake config files so that 
>>> `find_package(Clang)` would work in the first place? I'd also welcome your 
>>> input on the `ClangConfigVersion.cmake.in` changes.
>>>
>>> Just for the context, what we're trying to do here is simply use 
>>> `clang-tidy`'s development packages from the libc++ build to add our own 
>>> custom clang-tidy checks.
>>>
>>> Accepting because this LGTM, although we do have some stuff to resolve 
>>> before this can be shipped.
>>
>> IIRC, the intended solution was to use `LLVM_DISTRIBUTION_COMPONENTS` 
>> (https://llvm.org/docs/BuildingADistribution.html). When you use that 
>> option, the generated CMake package files (at least in the install tree; I'm 
>> not sure about the ones in the build tree) should only contain imported 
>> targets that were part of your distribution. Multi-distribution support 
>> extends this even further, where the file defining the imported targets for 
>> a distribution is only imported if it's present, so things should work as 
>> expected both with and without the dev packages. Is that workable for your 
>> use case?
>
> That's a thing the vendor has to change, right? If we only get the targets 
> which are actually available it should work just fine.

Correct, that would have to be set up by whoever distributes the LLVM package 
you're using.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131963

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

[PATCH] D135422: [clang-format] Fix misattributing preprocessor directives to macros

[Jacob Abraham via Phabricator via cfe-commits]

jacob-abraham updated this revision to Diff 466522.

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

https://reviews.llvm.org/D135422

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


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2999,6 +2999,10 @@
"}",
Style);
   Style.ColumnLimit = 21;
+  verifyFormat("#define X   \\\n"
+   "  case 0: break;\n"
+   "#include \"f\"",
+   Style);
   verifyFormat("switch (a) {\n"
"case 1: x = 1; break;\n"
"case 2: return;\n"
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -643,6 +643,7 @@
 unsigned Length = 0;
 bool EndsWithComment = false;
 bool InPPDirective = I[0]->InPPDirective;
+bool InMacroBody = I[0]->InMacroBody;
 const unsigned Level = I[0]->Level;
 for (; NumStmts < 3; ++NumStmts) {
   if (I + 1 + NumStmts == E)
@@ -650,6 +651,8 @@
   const AnnotatedLine *Line = I[1 + NumStmts];
   if (Line->InPPDirective != InPPDirective)
 break;
+  if(Line->InMacroBody != InMacroBody)
+break;
   if (Line->First->isOneOf(tok::kw_case, tok::kw_default, tok::r_brace))
 break;
   if (Line->First->isOneOf(tok::kw_if, tok::kw_for, tok::kw_switch,


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2999,6 +2999,10 @@
"}",
Style);
   Style.ColumnLimit = 21;
+  verifyFormat("#define X   \\\n"
+   "  case 0: break;\n"
+   "#include \"f\"",
+   Style);
   verifyFormat("switch (a) {\n"
"case 1: x = 1; break;\n"
"case 2: return;\n"
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -643,6 +643,7 @@
 unsigned Length = 0;
 bool EndsWithComment = false;
 bool InPPDirective = I[0]->InPPDirective;
+bool InMacroBody = I[0]->InMacroBody;
 const unsigned Level = I[0]->Level;
 for (; NumStmts < 3; ++NumStmts) {
   if (I + 1 + NumStmts == E)
@@ -650,6 +651,8 @@
   const AnnotatedLine *Line = I[1 + NumStmts];
   if (Line->InPPDirective != InPPDirective)
 break;
+  if(Line->InMacroBody != InMacroBody)
+break;
   if (Line->First->isOneOf(tok::kw_case, tok::kw_default, tok::r_brace))
 break;
   if (Line->First->isOneOf(tok::kw_if, tok::kw_for, tok::kw_switch,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135422: [clang-format] Fix misattributing preprocessor directives to macros

[Jacob Abraham via Phabricator via cfe-commits]

jacob-abraham added a comment.

I do not have commit access, can someone make this commit on my behalf? Thank 
you!


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

https://reviews.llvm.org/D135422

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

[PATCH] D135595: [HLSL] Add utility to convert environment to stage

[Xiang Li via Phabricator via cfe-commits]

python3kgae added a comment.

Backend needs the same thing.
Is it possible to move this to llvm and share it between frontend and backend?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135595

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

[clang] ec32386 - [Clang] Support constexpr builtin fmin

[Evgeny Shulgin via cfe-commits]

Author: Evgeny Shulgin
Date: 2022-10-10T16:06:23Z
New Revision: ec32386404409b65d21fdf916110c08912335926

URL: 
https://github.com/llvm/llvm-project/commit/ec32386404409b65d21fdf916110c08912335926
DIFF: 
https://github.com/llvm/llvm-project/commit/ec32386404409b65d21fdf916110c08912335926.diff

LOG: [Clang] Support constexpr builtin fmin

Support constexpr version of __builtin_fmin and its variations.

Reviewed By: jcranmer-intel

Differential Revision: https://reviews.llvm.org/D135493

Added: 
clang/test/Sema/constant-builtins-fmin.cpp

Modified: 
clang/docs/LanguageExtensions.rst
clang/lib/AST/ExprConstant.cpp

Removed: 




diff  --git a/clang/docs/LanguageExtensions.rst 
b/clang/docs/LanguageExtensions.rst
index 0793523d91e55..f680b1f1d2ad7 100644
--- a/clang/docs/LanguageExtensions.rst
+++ b/clang/docs/LanguageExtensions.rst
@@ -4661,6 +4661,7 @@ The following builtin intrinsics can be used in constant 
expressions:
 * ``__builtin_ffsl``
 * ``__builtin_ffsll``
 * ``__builtin_fmax``
+* ``__builtin_fmin``
 * ``__builtin_fpclassify``
 * ``__builtin_inf``
 * ``__builtin_isinf``

diff  --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index f100a4e19f8ad..64c50c8b82e5a 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -14041,6 +14041,24 @@ bool FloatExprEvaluator::VisitCallExpr(const CallExpr 
*E) {
   Result = RHS;
 return true;
   }
+
+  case Builtin::BI__builtin_fmin:
+  case Builtin::BI__builtin_fminf:
+  case Builtin::BI__builtin_fminl:
+  case Builtin::BI__builtin_fminf16:
+  case Builtin::BI__builtin_fminf128: {
+// TODO: Handle sNaN.
+APFloat RHS(0.);
+if (!EvaluateFloat(E->getArg(0), Result, Info) ||
+!EvaluateFloat(E->getArg(1), RHS, Info))
+  return false;
+// When comparing zeroes, return -0.0 if one of the zeroes is negative.
+if (Result.isZero() && RHS.isZero() && RHS.isNegative())
+  Result = RHS;
+else if (Result.isNaN() || RHS < Result)
+  Result = RHS;
+return true;
+  }
   }
 }
 

diff  --git a/clang/test/Sema/constant-builtins-fmin.cpp 
b/clang/test/Sema/constant-builtins-fmin.cpp
new file mode 100644
index 0..bf3e81e21e617
--- /dev/null
+++ b/clang/test/Sema/constant-builtins-fmin.cpp
@@ -0,0 +1,54 @@
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+constexpr double NaN = __builtin_nan("");
+constexpr double Inf = __builtin_inf();
+constexpr double NegInf = -__builtin_inf();
+
+#define FMIN_TEST_SIMPLE(T, FUNC)   \
+static_assert(T(1.2345) == FUNC(T(1.2345), T(6.7890))); \
+static_assert(T(1.2345) == FUNC(T(6.7890), T(1.2345)));
+
+#define FMIN_TEST_NAN(T, FUNC)  \
+static_assert(Inf == FUNC(NaN, Inf));   \
+static_assert(NegInf == FUNC(NegInf, NaN)); \
+static_assert(0.0 == FUNC(NaN, 0.0));   \
+static_assert(-0.0 == FUNC(-0.0, NaN)); \
+static_assert(T(-1.2345) == FUNC(NaN, T(-1.2345))); \
+static_assert(T(1.2345) == FUNC(T(1.2345), NaN));   \
+static_assert(__builtin_isnan(FUNC(NaN, NaN)));
+
+#define FMIN_TEST_INF(T, FUNC)\
+static_assert(NegInf == FUNC(NegInf, Inf));   \
+static_assert(0.0 == FUNC(Inf, 0.0)); \
+static_assert(-0.0 == FUNC(-0.0, Inf));   \
+static_assert(T(1.2345) == FUNC(Inf, T(1.2345))); \
+static_assert(T(-1.2345) == FUNC(T(-1.2345), Inf));
+
+#define FMIN_TEST_NEG_INF(T, FUNC) \
+static_assert(NegInf == FUNC(Inf, NegInf));\
+static_assert(NegInf == FUNC(NegInf, 0.0));\
+static_assert(NegInf == FUNC(-0.0, NegInf));   \
+static_assert(NegInf == FUNC(NegInf, T(-1.2345))); \
+static_assert(NegInf == FUNC(T(1.2345), NegInf));
+
+#define FMIN_TEST_BOTH_ZERO(T, FUNC) \
+static_assert(__builtin_copysign(1.0, FUNC(0.0, 0.0)) == 1.0);   \
+static_assert(__builtin_copysign(1.0, FUNC(-0.0, 0.0)) == -1.0); \
+static_assert(__builtin_copysign(1.0, FUNC(0.0, -0.0)) == -1.0); \
+static_assert(__builtin_copysign(1.0, FUNC(-0.0, -0.0)) == -1.0);
+
+#define LIST_FMIN_TESTS(T, FUNC) \
+FMIN_TEST_SIMPLE(T, FUNC)\
+FMIN_TEST_NAN(T, FUNC)   \
+FMIN_TEST_INF(T, FUNC)   \
+FMIN_TEST_NEG_INF(T, FUNC)   \
+FMIN_TEST_BOTH_ZERO(T, FUNC)
+
+LIST_FMIN_TESTS(double, __builtin_fmin)
+LIST_FMIN_TESTS(float, __builtin_fminf)
+LIST_FMIN_TESTS((long double), __builtin_fminl)
+LIST_FMIN_TESTS(__fp16, __builtin_fminf16)
+#ifdef __FLOAT128__
+LIST_FMIN_TESTS(__float128, __builtin_fminf128)
+#endif



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

[PATCH] D135493: [Clang] Support constexpr builtin fmin

[Evgeny Shulgin via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGec3238640440: [Clang] Support constexpr builtin fmin 
(authored by Izaron).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135493

Files:
  clang/docs/LanguageExtensions.rst
  clang/lib/AST/ExprConstant.cpp
  clang/test/Sema/constant-builtins-fmin.cpp


Index: clang/test/Sema/constant-builtins-fmin.cpp
===
--- /dev/null
+++ clang/test/Sema/constant-builtins-fmin.cpp
@@ -0,0 +1,54 @@
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+constexpr double NaN = __builtin_nan("");
+constexpr double Inf = __builtin_inf();
+constexpr double NegInf = -__builtin_inf();
+
+#define FMIN_TEST_SIMPLE(T, FUNC)   \
+static_assert(T(1.2345) == FUNC(T(1.2345), T(6.7890))); \
+static_assert(T(1.2345) == FUNC(T(6.7890), T(1.2345)));
+
+#define FMIN_TEST_NAN(T, FUNC)  \
+static_assert(Inf == FUNC(NaN, Inf));   \
+static_assert(NegInf == FUNC(NegInf, NaN)); \
+static_assert(0.0 == FUNC(NaN, 0.0));   \
+static_assert(-0.0 == FUNC(-0.0, NaN)); \
+static_assert(T(-1.2345) == FUNC(NaN, T(-1.2345))); \
+static_assert(T(1.2345) == FUNC(T(1.2345), NaN));   \
+static_assert(__builtin_isnan(FUNC(NaN, NaN)));
+
+#define FMIN_TEST_INF(T, FUNC)\
+static_assert(NegInf == FUNC(NegInf, Inf));   \
+static_assert(0.0 == FUNC(Inf, 0.0)); \
+static_assert(-0.0 == FUNC(-0.0, Inf));   \
+static_assert(T(1.2345) == FUNC(Inf, T(1.2345))); \
+static_assert(T(-1.2345) == FUNC(T(-1.2345), Inf));
+
+#define FMIN_TEST_NEG_INF(T, FUNC) \
+static_assert(NegInf == FUNC(Inf, NegInf));\
+static_assert(NegInf == FUNC(NegInf, 0.0));\
+static_assert(NegInf == FUNC(-0.0, NegInf));   \
+static_assert(NegInf == FUNC(NegInf, T(-1.2345))); \
+static_assert(NegInf == FUNC(T(1.2345), NegInf));
+
+#define FMIN_TEST_BOTH_ZERO(T, FUNC) \
+static_assert(__builtin_copysign(1.0, FUNC(0.0, 0.0)) == 1.0);   \
+static_assert(__builtin_copysign(1.0, FUNC(-0.0, 0.0)) == -1.0); \
+static_assert(__builtin_copysign(1.0, FUNC(0.0, -0.0)) == -1.0); \
+static_assert(__builtin_copysign(1.0, FUNC(-0.0, -0.0)) == -1.0);
+
+#define LIST_FMIN_TESTS(T, FUNC) \
+FMIN_TEST_SIMPLE(T, FUNC)\
+FMIN_TEST_NAN(T, FUNC)   \
+FMIN_TEST_INF(T, FUNC)   \
+FMIN_TEST_NEG_INF(T, FUNC)   \
+FMIN_TEST_BOTH_ZERO(T, FUNC)
+
+LIST_FMIN_TESTS(double, __builtin_fmin)
+LIST_FMIN_TESTS(float, __builtin_fminf)
+LIST_FMIN_TESTS((long double), __builtin_fminl)
+LIST_FMIN_TESTS(__fp16, __builtin_fminf16)
+#ifdef __FLOAT128__
+LIST_FMIN_TESTS(__float128, __builtin_fminf128)
+#endif
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -14041,6 +14041,24 @@
   Result = RHS;
 return true;
   }
+
+  case Builtin::BI__builtin_fmin:
+  case Builtin::BI__builtin_fminf:
+  case Builtin::BI__builtin_fminl:
+  case Builtin::BI__builtin_fminf16:
+  case Builtin::BI__builtin_fminf128: {
+// TODO: Handle sNaN.
+APFloat RHS(0.);
+if (!EvaluateFloat(E->getArg(0), Result, Info) ||
+!EvaluateFloat(E->getArg(1), RHS, Info))
+  return false;
+// When comparing zeroes, return -0.0 if one of the zeroes is negative.
+if (Result.isZero() && RHS.isZero() && RHS.isNegative())
+  Result = RHS;
+else if (Result.isNaN() || RHS < Result)
+  Result = RHS;
+return true;
+  }
   }
 }
 
Index: clang/docs/LanguageExtensions.rst
===
--- clang/docs/LanguageExtensions.rst
+++ clang/docs/LanguageExtensions.rst
@@ -4661,6 +4661,7 @@
 * ``__builtin_ffsl``
 * ``__builtin_ffsll``
 * ``__builtin_fmax``
+* ``__builtin_fmin``
 * ``__builtin_fpclassify``
 * ``__builtin_inf``
 * ``__builtin_isinf``


Index: clang/test/Sema/constant-builtins-fmin.cpp
===
--- /dev/null
+++ clang/test/Sema/constant-builtins-fmin.cpp
@@ -0,0 +1,54 @@
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+constexpr double NaN = __builtin_nan("");
+constexpr double Inf = __builtin_inf();
+constexpr double NegInf = -__builtin_inf();
+
+#define FMIN_TEST_SIMPLE(T, FUNC)   \
+static_assert(T(1.2345) == FUNC(T(1.2345), T(6.7890))); \
+static_assert(T(1.2345) == FUNC(T(6.7890), T(1.2345)));
+
+#define FMIN_TEST_NAN(T, FUNC)  \
+static_assert(Inf == FUNC(NaN, Inf));   \
+static_asser

[PATCH] D135595: [HLSL] Add utility to convert environment to stage

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

In D135595#3847056 , @python3kgae 
wrote:

> Backend needs the same thing.
> Is it possible to move this to llvm and share it between frontend and backend?

This translates the triple to a clang-defined enum... so strictly speaking no, 
that isn't possible.

Can you elaborate on the context where the backend needs this? I suspect in the 
backend we need a similar but not quite the same utility. Specifically, I think 
we need to translate the environment value to an unsigned integer. For all 
cases where an enum makes sense we should be using the Environment value in the 
backend.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135595

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

[PATCH] D133668: [HLSL] Use _BitInt(16) for int16_t to avoid promote to int.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Sure, but it's extremely easy to unpromote that arithmetic for most operators, 
and I'm sure LLVM already has a pass that will do that.  The only thing that 
blocks unpromotion is when you feed a promoted result into something that's 
sensitive to working on a wider value, like comparison or division, or of 
course an assignment to a wider type.

Alternatively, if you're worried about your ability to unpromote, and since 
you're breaking strict conformance anyway, have you considered just removing 
the initial promotion to `int` from the usual arithmetic conversions?  I'm 
pretty sure the rest of the rules hang together just fine if you do.  Then you 
have a uniform rule that permits easier vectorization for all small integer 
types rather than being sensitive specifically to using the `int16_t` typedef.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133668

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

[PATCH] D135595: [HLSL] Add utility to convert environment to stage

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

Worth noting, we do have a similar set of static_asserts in Triple.cpp to 
validate the ordering of enum cases and that the subtraction results in the 
appropriate values:

https://github.com/llvm/llvm-project/blob/main/llvm/lib/Support/Triple.cpp#L1942


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135595

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

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2029-2036
   // The align if the field is not packed. This is to check if the attribute
   // was unnecessary (-Wpacked).
   CharUnits UnpackedFieldAlign =
   !DefaultsToAIXPowerAlignment ? FieldAlign : PreferredAlign;
   CharUnits UnpackedFieldOffset = FieldOffset;
   CharUnits OriginalFieldAlign = UnpackedFieldAlign;
 

dblaikie wrote:
> dblaikie wrote:
> > rsmith wrote:
> > > It seems a little wasteful and error-prone that we're now computing the 
> > > actual alignment, the alignment if the field were not packed, and the 
> > > alignment if the field were packed. Is there any way we can reduce this 
> > > down to computing just the alignment if the field were packed plus the 
> > > alignment if the field were not packed, then picking one of those two as 
> > > the actual field alignment? Or does that end up being messier?
> > I had a go at that refactor - we can't pull the `FieldPacked` computation 
> > lower (it'd be great if it could move down to after the packed/unpacked 
> > computation, so it was clear that those values were computed independently 
> > of the `FieldPacked` value, and that `FieldPacked` just determined which 
> > one to pick) because of the `alignedAttrCanDecreaseAIXAlignment`, by the 
> > looks of it.
> > 
> > And also the AIX alignment stuff seems to do some weird things around the 
> > preferred alignment that caused the carefully constructed 3 `if`s below 
> > (`!FieldPacked`, `DefaultsToAIXPowerAlignment`, and `FieldPacked`) which I 
> > spent more time than I'd like to admit figuring out why anything 
> > else/less/more streamlined was inadequate.
> > 
> > But I also don't understand why `DefaultsToAIXAlignment` causes the 
> > `AlignTo` value to be the `PreferredAlign`, but the `FieldAlign` stays as 
> > it is? (like why doesn't `DefaultsToAIXPowerAlignment` cause `FieldAlign` 
> > to /be/ `PreferredAlign` - I think that'd simplify things, but tests (maybe 
> > the tests are incorrect/) seemed to break when I tried that) - I would've 
> > thought not doing that (as the code currently doesn't) would cause problems 
> > for the `UnadjustedAlignment`, `UpdateAlignment`, and 
> > `warn_unaligned_access` issues later on that depend on `FieldAlign`, but 
> > feel like they should probably depend on the alignment that actually got 
> > used (the `PreferredAlign`) instead? It's pretty confusing to me, so... 
> > yeah.
> Ping on this discussion. @rsmith 
@rsmith ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118511

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

[PATCH] D135433: [clang][Interp] Implement while and do-while loops

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/test/AST/Interp/loops.cpp:9
+
+namespace WhileLoop {
+  constexpr int f() {

tbaeder wrote:
> shafik wrote:
> > infinite loop w/o a side effect are undefined behavior and so should be 
> > ill-formed and generate a diagnostic e.g. `while(1);`, so we should check 
> > these cases. 
> I think that's better done with a more general approach that limits the 
> iteration count for all loops like the current interpreter does.  But that 
> would probably blow up this patch too much.
> 
> Unfortunately I can't add test case for the case you describe because the 
> clang process with the new interpreter would never terminate :) Can add a 
> commented-out version though.
I think that is fine approach for now.


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

https://reviews.llvm.org/D135433

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

[PATCH] D135341: [clang] adds `__reference_constructs_from_temporary`

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:397
+def err_reserved_identifier_for_future_use : Error<
+  "%0 is a compiler-reserved identifier for a future feature">;
 }

erichkeane wrote:
> I don't think we should diagnose this for individual identifiers, I don't 
> think this adds much value, and the identifier is already reserved for 
> implementation use.  This implies we are going to diagnose ALL future 
> reserved things, which we have no intention of doing.
The motivation for this error is to ensure that standard library 
implementations don't take this name (`__remove_cvref` and `__is_null_pointer` 
had to get names that weren't a 1:1 mapping to their library counterparts as a 
result).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135341

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

[PATCH] D135177: [clang] adds `__is_scoped_enum`, `__is_nullptr`, and `__is_referenceable`

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added inline comments.



Comment at: clang/lib/Parse/ParseExpr.cpp:1071
   REVERTIBLE_TYPE_TRAIT(__is_base_of);
+  REVERTIBLE_TYPE_TRAIT(__is_bounded_array);
   REVERTIBLE_TYPE_TRAIT(__is_class);

erichkeane wrote:
> Can you just put this in the right place in the other commit?
Yeah, this feels like a commit split issue. Thanks for catching.



Comment at: clang/test/SemaCXX/type-traits.cpp:374
 
+void is_scoped_enum() {
+  static_assert(!__is_scoped_enum(Enum), "");

aaron.ballman wrote:
> It'd probably not be a bad idea at some point to add test coverage for 
> incomplete types.
I've added that coverage for `__is_enum` above too.



Comment at: clang/test/SemaCXX/type-traits.cpp:822
+
+  { int a[F(__is_referenceable(void))]; }
+}

aaron.ballman wrote:
> I think we should have test cases for the following:
> ```
> struct incomplete;
> 
> __is_referenceable(struct incomplete); // Also interesting to make sure we 
> handle elaborated type specifiers properly
> 
> typedef void function_type(int);
> __is_referenceable(function_type); // Note, this is not a function *pointer* 
> type
> 
> struct S {
>   void func1() &;
>   void func2() const;
> };
> 
> // Both of these should be false, by my understanding of what "referenceable" 
> means in the standard.
> __is_referenceable(decltype(&S::func1));
> __is_referenceable(decltype(&S::func2));
> ```
I think the latter two are pointer-to-member functions, which are 
referenceable. We want to check `void(int) const`, which is done on the last 
green line of this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135177

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

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

[YingChi Long via Phabricator via cfe-commits]

inclyc updated this revision to Diff 466539.
inclyc added a comment.

Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135551

Files:
  clang/include/clang/AST/Redeclarable.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/Interp/ByteCodeExprGen.cpp
  clang/lib/Analysis/CFG.cpp
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Basic/Targets/NVPTX.cpp
  clang/lib/CodeGen/CGHLSLRuntime.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/Multilib.cpp
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Frontend/Rewrite/RewriteModernObjC.cpp
  clang/lib/Frontend/Rewrite/RewriteObjC.cpp
  clang/lib/Frontend/SARIFDiagnostic.cpp
  clang/lib/Lex/PPDirectives.cpp
  clang/lib/Lex/PreprocessingRecord.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
  clang/lib/StaticAnalyzer/Core/SVals.cpp
  clang/tools/clang-refactor/TestSupport.cpp

Index: clang/tools/clang-refactor/TestSupport.cpp
===
--- clang/tools/clang-refactor/TestSupport.cpp
+++ clang/tools/clang-refactor/TestSupport.cpp
@@ -348,7 +348,7 @@
 if (!Matches[2].empty()) {
   // Don't forget to drop the '+'!
   if (Matches[2].drop_front().getAsInteger(10, ColumnOffset))
-assert(false && "regex should have produced a number");
+llvm_unreachable("regex should have produced a number");
 }
 Offset = addColumnOffset(Source, Offset, ColumnOffset);
 unsigned EndOffset;
@@ -365,7 +365,7 @@
   unsigned EndLineOffset = 0, EndColumn = 0;
   if (EndLocMatches[1].drop_front().getAsInteger(10, EndLineOffset) ||
   EndLocMatches[2].getAsInteger(10, EndColumn))
-assert(false && "regex should have produced a number");
+llvm_unreachable("regex should have produced a number");
   EndOffset = addEndLineOffsetAndEndColumn(Source, Offset, EndLineOffset,
EndColumn);
 } else {
Index: clang/lib/StaticAnalyzer/Core/SVals.cpp
===
--- clang/lib/StaticAnalyzer/Core/SVals.cpp
+++ clang/lib/StaticAnalyzer/Core/SVals.cpp
@@ -355,7 +355,7 @@
   break;
 }
 default:
-  assert(false && "Pretty-printed not implemented for this NonLoc.");
+  llvm_unreachable("Pretty-printed not implemented for this NonLoc.");
   break;
   }
 }
Index: clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -192,7 +192,7 @@
   break;
 
 case ProgramPoint::BlockExitKind:
-  assert(false && "BlockExit location never occur in forward analysis.");
+  llvm_unreachable("BlockExit location never occur in forward analysis.");
   break;
 
 case ProgramPoint::CallEnterKind:
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -2438,7 +2438,7 @@
 MacroID ID = MacroInfosToEmit[I].ID;
 
 if (ID < FirstMacroID) {
-  assert(0 && "Loaded MacroInfo entered MacroInfosToEmit ?");
+  llvm_unreachable("Loaded MacroInfo entered MacroInfosToEmit ?");
   continue;
 }
 
@@ -5381,7 +5381,8 @@
 TypeIdx &Idx = TypeIdxs[T];
 if (Idx.getIndex() == 0) {
   if (DoneWritingDeclsAndTypes) {
-assert(0 && "New type seen after serializing all the types to emit!");
+llvm_unreachable(
+"New type seen after serializing all the types to emit!");
 return TypeIdx();
   }
 
@@ -5427,7 +5428,8 @@
   DeclID &ID = DeclIDs[D];
   if (ID == 0) {
 if (DoneWritingDeclsAndTypes) {
-  assert(0 && "New decl seen after serializing all the decls to emit!");
+  llvm_unreachable(
+  "New decl seen after serializing all the decls to emit!");
   return 0;
 }
 
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -7464,7 +7464,7 @@
   unsigned Index = ID - NUM_PREDEF_DECL_IDS;
 
   if (Index >= DeclsLoaded.size()) {
-assert(0 && "declaration ID out-of-range for AST file");
+llvm_unreachable("declaration ID out-of-range for AST file");
 Error("declaration ID out-of-range for AST file");
 return nullptr;
   }
@@ -7479,7 +7479,7 @@
   unsigned Index = ID - NUM_PREDEF_DECL_IDS;
 
   if (Index >= DeclsLoaded

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

[YingChi Long via Phabricator via cfe-commits]

inclyc added a comment.

> I've left some comments in the review about examples of my concerns (it's not 
> an exhaustive review).

Thanks @aaron.ballman ! I didn't quite understand the original meaning of this 
code here (e.g. libclang), and I have now removed the relevant changes. I think 
this patch should replace the code that accidentally misuses of `assert(0)` 
with `llvm_unreachable()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135551

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

[PATCH] D127403: [clangd] Implement semantic token modifier "definition"

[Sam McCall via Phabricator via cfe-commits]

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

Just realized ParmVarDecls (and template args) are maybe more complicated than 
I thought: I suppose they're only really definitions if the function/template 
has a body (i.e. is itself a definition).
Anyway don't worry about that, let's get this landed and maybe refine later.




Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:846
+void $Function_decl[[Foo]](int $Parameter_def[[x]]);
 template 
+void $Function_def[[Bar]]($TemplateParameter[[T]] $Parameter_def[[x]]) 
{

nit: T should be a def here too.
it's not critical so if this case is hard at all feel free to leave it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127403

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

[PATCH] D135177: [clang] adds `__is_scoped_enum`, `__is_nullptr`, and `__is_referenceable`

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM




Comment at: clang/test/SemaCXX/type-traits.cpp:822
+
+  { int a[F(__is_referenceable(void))]; }
+}

aaron.ballman wrote:
> cjdb wrote:
> > aaron.ballman wrote:
> > > I think we should have test cases for the following:
> > > ```
> > > struct incomplete;
> > > 
> > > __is_referenceable(struct incomplete); // Also interesting to make sure 
> > > we handle elaborated type specifiers properly
> > > 
> > > typedef void function_type(int);
> > > __is_referenceable(function_type); // Note, this is not a function 
> > > *pointer* type
> > > 
> > > struct S {
> > >   void func1() &;
> > >   void func2() const;
> > > };
> > > 
> > > // Both of these should be false, by my understanding of what 
> > > "referenceable" means in the standard.
> > > __is_referenceable(decltype(&S::func1));
> > > __is_referenceable(decltype(&S::func2));
> > > ```
> > I think the latter two are pointer-to-member functions, which are 
> > referenceable. We want to check `void(int) const`, which is done on the 
> > last green line of this file.
> Do you have a test for:
> ```
> struct S {
>   void func1() &;
>   void func2() const;
> };
> 
> // Both of these should be false, by my understanding of what "referenceable" 
> means in the standard.
> __is_referenceable(decltype(&S::func1));
> __is_referenceable(decltype(&S::func2));
> ```
> 
Ah thank you for showing me we already had the coverage!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135177

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

[PATCH] D130325: [ODRHash] Hash `ObjCMethodDecl` and diagnose discovered mismatches.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno accepted this revision.
bruno added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130325

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

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

[Emilia Dreamer via Phabricator via cfe-commits]

rymiel updated this revision to Diff 466551.
rymiel added a comment.

Flip the default and note that in the Release Notes

Sorry, I did not realize I was mentioned.
I think the default option will always be the first option? So I swapped them 
around.
Note that I didn't change the LLVM config, which right now is still set to 
`Keyword`.
I'm also not sure if the release notes should note this as "important" or 
"breaking"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129443

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/ContinuationIndenter.cpp
  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
@@ -24995,6 +24995,123 @@
"bar(requires);");
 }
 
+TEST_F(FormatTest, RequiresExpressionIndentation) {
+  auto Style = getLLVMStyle();
+  EXPECT_EQ(Style.RequiresExpressionIndentation, FormatStyle::REI_Keyword);
+
+  verifyFormat("template \n"
+   "concept C = requires(T t) {\n"
+   "  typename T::value;\n"
+   "  requires requires(typename T::value v) {\n"
+   " { t == v } -> std::same_as;\n"
+   "   };\n"
+   "};",
+   Style);
+
+  verifyFormat(
+  "template \n"
+  "void bar(T)\n"
+  "  requires Foo && requires(T t) {\n"
+  "   { t.foo() } -> std::same_as;\n"
+  " } && requires(T t) {\n"
+  "{ t.bar() } -> std::same_as;\n"
+  "--t;\n"
+  "  };",
+  Style);
+
+  verifyFormat("template \n"
+   "  requires Foo &&\n"
+   "   requires(T t) {\n"
+   " { t.foo() } -> std::same_as;\n"
+   "   } && requires(T t) {\n"
+   "  { t.bar() } -> std::same_as;\n"
+   "  --t;\n"
+   "}\n"
+   "void bar(T);",
+   Style);
+
+  verifyFormat("template  void f() {\n"
+   "  if constexpr (requires(T t) {\n"
+   "  { t.bar() } -> std::same_as;\n"
+   "}) {\n"
+   "  }\n"
+   "}",
+   Style);
+
+  verifyFormat(
+  "template  void f() {\n"
+  "  if constexpr (condition && requires(T t) {\n"
+  "   { t.bar() } -> std::same_as;\n"
+  " }) {\n"
+  "  }\n"
+  "}",
+  Style);
+
+  verifyFormat("template  struct C {\n"
+   "  void f()\n"
+   "requires requires(T t) {\n"
+   "   { t.bar() } -> std::same_as;\n"
+   " };\n"
+   "};",
+   Style);
+
+  Style.RequiresExpressionIndentation = FormatStyle::REI_OuterScope;
+
+  verifyFormat("template \n"
+   "concept C = requires(T t) {\n"
+   "  typename T::value;\n"
+   "  requires requires(typename T::value v) {\n"
+   "{ t == v } -> std::same_as;\n"
+   "  };\n"
+   "};",
+   Style);
+
+  verifyFormat("template \n"
+   "void bar(T)\n"
+   "  requires Foo && requires(T t) {\n"
+   "{ t.foo() } -> std::same_as;\n"
+   "  } && requires(T t) {\n"
+   "{ t.bar() } -> std::same_as;\n"
+   "--t;\n"
+   "  };",
+   Style);
+
+  verifyFormat("template \n"
+   "  requires Foo &&\n"
+   "   requires(T t) {\n"
+   " { t.foo() } -> std::same_as;\n"
+   "   } && requires(T t) {\n"
+   " { t.bar() } -> std::same_as;\n"
+   " --t;\n"
+   "   }\n"
+   "void bar(T);",
+   Style);
+
+  verifyFormat("template  void f() {\n"
+   "  if constexpr (requires(T t) {\n"
+   "  { t.bar() } -> std::same_as;\n"
+   "}) {\n"
+   "  }\n"
+   "}",
+   Style);
+
+  verifyFormat("template  void f() {\n"
+   "  if constexpr (condition && requires(T t) {\n"
+   "  { t.bar() } -> std::same_as;\n"
+   "}) {\n"
+   "  }\n"
+   "}",
+   Style);
+
+  verifyFormat("template  struct C {\n"
+   "

Re: [clang-tools-extra] 5d2d527 - [clangd] Avoid scanning up to end of file on each comment!

[David Blaikie via cfe-commits]

Could the underlying API be made more robust to handle StringRefs
rather than null terminated char* in the first place? (like
SourceManager::getCharacterData could return StringRef? Presumably at
some layer it already knows how long the file is - the underlying
MemoryBuffer, in this case)

On Thu, Oct 6, 2022 at 2:39 AM Sam McCall via cfe-commits
 wrote:
>
>
> Author: Sam McCall
> Date: 2022-10-06T11:38:55+02:00
> New Revision: 5d2d527c32da2081b814ef8b446bc3e037f74b0a
>
> URL: 
> https://github.com/llvm/llvm-project/commit/5d2d527c32da2081b814ef8b446bc3e037f74b0a
> DIFF: 
> https://github.com/llvm/llvm-project/commit/5d2d527c32da2081b814ef8b446bc3e037f74b0a.diff
>
> LOG: [clangd] Avoid scanning up to end of file on each comment!
>
> Assigning char* (pointing at comment start) to StringRef was causing us
> to scan the rest of the source file looking for the null terminator.
>
> This seems to be eating about 8% of our *total* CPU!
>
> While fixing this, factor out the common bits from the two places we're
> parsing IWYU pragmas.
>
> Differential Revision: https://reviews.llvm.org/D135314
>
> Added:
>
>
> Modified:
> clang-tools-extra/clangd/Headers.cpp
> clang-tools-extra/clangd/Headers.h
> clang-tools-extra/clangd/index/CanonicalIncludes.cpp
> clang-tools-extra/clangd/unittests/HeadersTests.cpp
>
> Removed:
>
>
>
> 
> diff  --git a/clang-tools-extra/clangd/Headers.cpp 
> b/clang-tools-extra/clangd/Headers.cpp
> index 5231a47487bc7..52b954e921620 100644
> --- a/clang-tools-extra/clangd/Headers.cpp
> +++ b/clang-tools-extra/clangd/Headers.cpp
> @@ -22,9 +22,17 @@
>  namespace clang {
>  namespace clangd {
>
> -const char IWYUPragmaKeep[] = "// IWYU pragma: keep";
> -const char IWYUPragmaExport[] = "// IWYU pragma: export";
> -const char IWYUPragmaBeginExports[] = "// IWYU pragma: begin_exports";
> +llvm::Optional parseIWYUPragma(const char *Text) {
> +  // This gets called for every comment seen in the preamble, so it's quite 
> hot.
> +  constexpr llvm::StringLiteral IWYUPragma = "// IWYU pragma: ";
> +  if (strncmp(Text, IWYUPragma.data(), IWYUPragma.size()))
> +return llvm::None;
> +  Text += IWYUPragma.size();
> +  const char *End = Text;
> +  while (*End != 0 && *End != '\n')
> +++End;
> +  return StringRef(Text, End - Text);
> +}
>
>  class IncludeStructure::RecordHeaders : public PPCallbacks,
>  public CommentHandler {
> @@ -129,10 +137,10 @@ class IncludeStructure::RecordHeaders : public 
> PPCallbacks,
>}
>
>bool HandleComment(Preprocessor &PP, SourceRange Range) override {
> -bool Err = false;
> -llvm::StringRef Text = SM.getCharacterData(Range.getBegin(), &Err);
> -if (Err)
> +auto Pragma = parseIWYUPragma(SM.getCharacterData(Range.getBegin()));
> +if (!Pragma)
>return false;
> +
>  if (inMainFile()) {
>// Given:
>//
> @@ -150,8 +158,7 @@ class IncludeStructure::RecordHeaders : public 
> PPCallbacks,
>// will know that the next inclusion is behind the IWYU pragma.
>// FIXME: Support "IWYU pragma: begin_exports" and "IWYU pragma:
>// end_exports".
> -  if (!Text.startswith(IWYUPragmaExport) &&
> -  !Text.startswith(IWYUPragmaKeep))
> +  if (!Pragma->startswith("export") && !Pragma->startswith("keep"))
>  return false;
>unsigned Offset = SM.getFileOffset(Range.getBegin());
>LastPragmaKeepInMainFileLine =
> @@ -161,8 +168,7 @@ class IncludeStructure::RecordHeaders : public 
> PPCallbacks,
>// does not support them properly yet, so they will be not marked as
>// unused.
>// FIXME: Once IncludeCleaner supports export pragmas, remove this.
> -  if (!Text.startswith(IWYUPragmaExport) &&
> -  !Text.startswith(IWYUPragmaBeginExports))
> +  if (!Pragma->startswith("export") && 
> !Pragma->startswith("begin_exports"))
>  return false;
>Out->HasIWYUExport.insert(
>*Out->getID(SM.getFileEntryForID(SM.getFileID(Range.getBegin();
>
> diff  --git a/clang-tools-extra/clangd/Headers.h 
> b/clang-tools-extra/clangd/Headers.h
> index ff3f063168325..ba72ad397bf8f 100644
> --- a/clang-tools-extra/clangd/Headers.h
> +++ b/clang-tools-extra/clangd/Headers.h
> @@ -35,6 +35,12 @@ namespace clangd {
>  /// Returns true if \p Include is literal include like "path" or .
>  bool isLiteralInclude(llvm::StringRef Include);
>
> +/// If Text begins an Include-What-You-Use directive, returns it.
> +/// Given "// IWYU pragma: keep", returns "keep".
> +/// Input is a null-terminated char* as provided by SM.getCharacterData().
> +/// (This should not be StringRef as we do *not* want to scan for its 
> length).
> +llvm::Optional parseIWYUPragma(const char *Text);
> +
>  /// Represents a header file to be #include'd.
>  struct HeaderFile {
>std::string File;
>
> diff  --git a/clang-tools-extra/clangd/ind

[clang] 9ced729 - Repair a confusing standards reference; NFC

[Aaron Ballman via cfe-commits]

Author: Aaron Ballman
Date: 2022-10-10T14:10:39-04:00
New Revision: 9ced729c2c77b19932966891566e4e2f01112f91

URL: 
https://github.com/llvm/llvm-project/commit/9ced729c2c77b19932966891566e4e2f01112f91
DIFF: 
https://github.com/llvm/llvm-project/commit/9ced729c2c77b19932966891566e4e2f01112f91.diff

LOG: Repair a confusing standards reference; NFC

There is no 6.9 in C++11, the quote actually lives in
[intro.multithread] for that revision. However, the words moved in
C++17 to [intro.progress] so I added that information as well.

Added: 


Modified: 
clang/lib/CodeGen/CodeGenFunction.h

Removed: 




diff  --git a/clang/lib/CodeGen/CodeGenFunction.h 
b/clang/lib/CodeGen/CodeGenFunction.h
index b906fd0d1c121..8921cb78de996 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -570,7 +570,7 @@ class CodeGenFunction : public CodeGenTypeCache {
   return false;
 
 // C++11 and later guarantees that a thread eventually will do one of the
-// following (6.9.2.3.1 in C++11):
+// following (C++11 [intro.multithread]p24 and C++17 [intro.progress]p1):
 // - terminate,
 //  - make a call to a library I/O function,
 //  - perform an access through a volatile glvalue, or



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

[PATCH] D119209: [clang] Add cc1 option -fctor-dtor-return-this

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

I was rather hoping we could have a more general "floating ABI" flag that we 
can lump in anything that we'd like to have otherwise but can't break ABI for - 
so users like Fuschia that know they're compiling everything with the same 
version of Clang can opt into the floating ABI & get all the benefits whenever 
they come up, like we would with any other non-ABI impacting optimization.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119209

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

Re: [clang-tools-extra] 5d2d527 - [clangd] Avoid scanning up to end of file on each comment!

[Sam McCall via cfe-commits]

On Mon, 10 Oct 2022, 19:57 David Blaikie,  wrote:

> Could the underlying API be made more robust to handle StringRefs
> rather than null terminated char* in the first place? (like
> SourceManager::getCharacterData could return StringRef? Presumably at
> some layer it already knows how long the file is - the underlying
> MemoryBuffer, in this case)
>
Maybe...

You could return a StringRef(Pos, EOF) efficiently.
However it's not a great fit for many of the existing (>150) callers:
 - that are looking for the *endpoint* of some text
 - which want to run the lexer (the returned char* is implicitly
null-terminated, StringRef would obscure that guarantee)
 - the function is marked as "this is very hot", though I doubt it matters
that much
It also leads to some unnatural code at callsites such as this, because
"from the start of this entity until the end of the file" is an unnatural
range to have, it would probably be easy to mistake for "from the start to
the end of this entity".

I think what this function does is too low-level to make safe and obvious,
and unfortunately it's hard to build high-level things on top of it.
(e.g. StringRef getTextRange(SourceRange) would be great to have, but
actually you it needs to run the lexer and assert nontrivial invariants due
to design decisions elsewhere in clang)


> On Thu, Oct 6, 2022 at 2:39 AM Sam McCall via cfe-commits
>  wrote:
> >
> >
> > Author: Sam McCall
> > Date: 2022-10-06T11:38:55+02:00
> > New Revision: 5d2d527c32da2081b814ef8b446bc3e037f74b0a
> >
> > URL:
> https://github.com/llvm/llvm-project/commit/5d2d527c32da2081b814ef8b446bc3e037f74b0a
> > DIFF:
> https://github.com/llvm/llvm-project/commit/5d2d527c32da2081b814ef8b446bc3e037f74b0a.diff
> >
> > LOG: [clangd] Avoid scanning up to end of file on each comment!
> >
> > Assigning char* (pointing at comment start) to StringRef was causing us
> > to scan the rest of the source file looking for the null terminator.
> >
> > This seems to be eating about 8% of our *total* CPU!
> >
> > While fixing this, factor out the common bits from the two places we're
> > parsing IWYU pragmas.
> >
> > Differential Revision: https://reviews.llvm.org/D135314
> >
> > Added:
> >
> >
> > Modified:
> > clang-tools-extra/clangd/Headers.cpp
> > clang-tools-extra/clangd/Headers.h
> > clang-tools-extra/clangd/index/CanonicalIncludes.cpp
> > clang-tools-extra/clangd/unittests/HeadersTests.cpp
> >
> > Removed:
> >
> >
> >
> >
> 
> > diff  --git a/clang-tools-extra/clangd/Headers.cpp
> b/clang-tools-extra/clangd/Headers.cpp
> > index 5231a47487bc7..52b954e921620 100644
> > --- a/clang-tools-extra/clangd/Headers.cpp
> > +++ b/clang-tools-extra/clangd/Headers.cpp
> > @@ -22,9 +22,17 @@
> >  namespace clang {
> >  namespace clangd {
> >
> > -const char IWYUPragmaKeep[] = "// IWYU pragma: keep";
> > -const char IWYUPragmaExport[] = "// IWYU pragma: export";
> > -const char IWYUPragmaBeginExports[] = "// IWYU pragma: begin_exports";
> > +llvm::Optional parseIWYUPragma(const char *Text) {
> > +  // This gets called for every comment seen in the preamble, so it's
> quite hot.
> > +  constexpr llvm::StringLiteral IWYUPragma = "// IWYU pragma: ";
> > +  if (strncmp(Text, IWYUPragma.data(), IWYUPragma.size()))
> > +return llvm::None;
> > +  Text += IWYUPragma.size();
> > +  const char *End = Text;
> > +  while (*End != 0 && *End != '\n')
> > +++End;
> > +  return StringRef(Text, End - Text);
> > +}
> >
> >  class IncludeStructure::RecordHeaders : public PPCallbacks,
> >  public CommentHandler {
> > @@ -129,10 +137,10 @@ class IncludeStructure::RecordHeaders : public
> PPCallbacks,
> >}
> >
> >bool HandleComment(Preprocessor &PP, SourceRange Range) override {
> > -bool Err = false;
> > -llvm::StringRef Text = SM.getCharacterData(Range.getBegin(), &Err);
> > -if (Err)
> > +auto Pragma =
> parseIWYUPragma(SM.getCharacterData(Range.getBegin()));
> > +if (!Pragma)
> >return false;
> > +
> >  if (inMainFile()) {
> >// Given:
> >//
> > @@ -150,8 +158,7 @@ class IncludeStructure::RecordHeaders : public
> PPCallbacks,
> >// will know that the next inclusion is behind the IWYU pragma.
> >// FIXME: Support "IWYU pragma: begin_exports" and "IWYU pragma:
> >// end_exports".
> > -  if (!Text.startswith(IWYUPragmaExport) &&
> > -  !Text.startswith(IWYUPragmaKeep))
> > +  if (!Pragma->startswith("export") && !Pragma->startswith("keep"))
> >  return false;
> >unsigned Offset = SM.getFileOffset(Range.getBegin());
> >LastPragmaKeepInMainFileLine =
> > @@ -161,8 +168,7 @@ class IncludeStructure::RecordHeaders : public
> PPCallbacks,
> >// does not support them properly yet, so they will be not marked
> as
> >// unused.
> >// FIXME: Once IncludeCleaner supports export pragmas, remove

[clang] 231fc00 - [clang-format][docs][NFC] Fix invalid syntax in ShortLambdaStyle examples.

[Emilia Dreamer via cfe-commits]

Author: Emilia Dreamer
Date: 2022-10-10T21:13:07+03:00
New Revision: 231fc00cebc127badb6df6e8902727e893335a58

URL: 
https://github.com/llvm/llvm-project/commit/231fc00cebc127badb6df6e8902727e893335a58
DIFF: 
https://github.com/llvm/llvm-project/commit/231fc00cebc127badb6df6e8902727e893335a58.diff

LOG: [clang-format][docs][NFC] Fix invalid syntax in ShortLambdaStyle examples.

Added: 


Modified: 
clang/docs/ClangFormatStyleOptions.rst
clang/include/clang/Format/Format.h

Removed: 




diff  --git a/clang/docs/ClangFormatStyleOptions.rst 
b/clang/docs/ClangFormatStyleOptions.rst
index ec53c3eef8540..cdbc23ae43eb8 100644
--- a/clang/docs/ClangFormatStyleOptions.rst
+++ b/clang/docs/ClangFormatStyleOptions.rst
@@ -,7 +,7 @@ the configuration (without a prefix: ``Auto``).
 
 .. code-block:: c++
 
-  auto lambda = [](int a) {}
+  auto lambda = [](int a) {};
   auto lambda2 = [](int a) {
   return a;
   };
@@ -1124,14 +1124,14 @@ the configuration (without a prefix: ``Auto``).
   auto lambda = [](int a) {
   return a;
   };
-  sort(a.begin(), a.end(), ()[] { return x < y; })
+  sort(a.begin(), a.end(), []() { return x < y; });
 
   * ``SLS_All`` (in configuration: ``All``)
 Merge all lambdas fitting on a single line.
 
 .. code-block:: c++
 
-  auto lambda = [](int a) {}
+  auto lambda = [](int a) {};
   auto lambda2 = [](int a) { return a; };
 
 

diff  --git a/clang/include/clang/Format/Format.h 
b/clang/include/clang/Format/Format.h
index ee1863a63bc9a..7c1721064727c 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -610,7 +610,7 @@ struct FormatStyle {
 SLS_None,
 /// Only merge empty lambdas.
 /// \code
-///   auto lambda = [](int a) {}
+///   auto lambda = [](int a) {};
 ///   auto lambda2 = [](int a) {
 ///   return a;
 ///   };
@@ -621,12 +621,12 @@ struct FormatStyle {
 ///   auto lambda = [](int a) {
 ///   return a;
 ///   };
-///   sort(a.begin(), a.end(), ()[] { return x < y; })
+///   sort(a.begin(), a.end(), []() { return x < y; });
 /// \endcode
 SLS_Inline,
 /// Merge all lambdas fitting on a single line.
 /// \code
-///   auto lambda = [](int a) {}
+///   auto lambda = [](int a) {};
 ///   auto lambda2 = [](int a) { return a; };
 /// \endcode
 SLS_All,



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

[PATCH] D129156: Add -fpass-plugin option to Flang

[Tarun Prabhu via Phabricator via cfe-commits]

tarunprabhu updated this revision to Diff 466561.
tarunprabhu added a comment.

Added `examples` to `REQUIRES` in `test/Driver/pass-plugin.f90.

I still cannot reproduce the build failure on my end. @MatsPetersson tested 
this patch and the tests passed. Could someone else test this on their build 
and let me know if it works - especially if the previous version failed for you.


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

https://reviews.llvm.org/D129156

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/docs/FlangDriver.md
  flang/docs/ReleaseNotes.md
  flang/include/flang/Frontend/CodeGenOptions.h
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-help.f90
  flang/test/Driver/frontend-forwarding.f90
  flang/test/Driver/pass-plugin-not-found.f90
  flang/test/Driver/pass-plugin.f90

Index: flang/test/Driver/pass-plugin.f90
===
--- /dev/null
+++ flang/test/Driver/pass-plugin.f90
@@ -0,0 +1,13 @@
+! Verify that the plugin passed to -fpass-plugin is loaded and run
+
+! UNSUPPORTED: system-windows
+
+! REQUIRES: examples, plugins, shell
+
+! RUN: %flang -S %s -fpass-plugin=%llvmshlibdir/Bye%pluginext -Xflang -fdebug-pass-manager -o /dev/null 2>&1 | FileCheck %s
+! RUN: %flang_fc1 -S %s -fpass-plugin=%llvmshlibdir/Bye%pluginext -fdebug-pass-manager -o /dev/null 2>&1 | FileCheck %s
+
+! CHECK: Running pass: {{.*}}Bye on empty_
+
+subroutine empty
+end subroutine empty
Index: flang/test/Driver/pass-plugin-not-found.f90
===
--- /dev/null
+++ flang/test/Driver/pass-plugin-not-found.f90
@@ -0,0 +1,8 @@
+! Check the correct error diagnostic is reported when a pass plugin shared object isn't found
+
+! REQUIRES: plugins, shell
+
+! RUN: not %flang -fpass-plugin=X.Y %s 2>&1 | FileCheck %s --check-prefix=ERROR
+! RUN: not %flang_fc1 -emit-llvm -o /dev/null -fpass-plugin=X.Y %s 2>&1 | FileCheck %s --check-prefix=ERROR
+
+! ERROR: error: unable to load plugin 'X.Y': 'Could not load library 'X.Y': X.Y: cannot open shared object file: No such file or directory'
Index: flang/test/Driver/frontend-forwarding.f90
===
--- flang/test/Driver/frontend-forwarding.f90
+++ flang/test/Driver/frontend-forwarding.f90
@@ -7,6 +7,7 @@
 ! RUN: -fdefault-integer-8 \
 ! RUN: -fdefault-real-8 \
 ! RUN: -flarge-sizes \
+! RUN: -fpass-plugin=Bye%pluginext \
 ! RUN: -mllvm -print-before-all\
 ! RUN: -P \
 ! RUN:   | FileCheck %s
@@ -17,4 +18,5 @@
 ! CHECK: "-fdefault-integer-8"
 ! CHECK: "-fdefault-real-8"
 ! CHECK: "-flarge-sizes"
-! CHECK:  "-mllvm" "-print-before-all"
+! CHECK: "-fpass-plugin=Bye
+! CHECK: "-mllvm" "-print-before-all"
Index: flang/test/Driver/driver-help.f90
===
--- flang/test/Driver/driver-help.f90
+++ flang/test/Driver/driver-help.f90
@@ -42,6 +42,7 @@
 ! HELP-NEXT: -fno-integrated-as  Disable the integrated assembler
 ! HELP-NEXT: -fopenacc  Enable OpenACC
 ! HELP-NEXT: -fopenmp   Parse OpenMP pragmas and generate parallel code.
+! HELP-NEXT: -fpass-plugin= Load pass plugin from a dynamic shared object file (only with new pass manager).
 ! HELP-NEXT: -fsyntax-only  Run the preprocessor, parser and semantic analysis stages
 ! HELP-NEXT: -fxor-operator Enable .XOR. as a synonym of .NEQV.
 ! HELP-NEXT: -help  Display available options
@@ -120,6 +121,7 @@
 ! HELP-FC1-NEXT: -fno-reformat  Dump the cooked character stream in -E mode
 ! HELP-FC1-NEXT: -fopenacc  Enable OpenACC
 ! HELP-FC1-NEXT: -fopenmp   Parse OpenMP pragmas and generate parallel code.
+! HELP-FC1-NEXT: -fpass-plugin= Load pass plugin from a dynamic shared object file (only with new pass manager).
 ! HELP-FC1-NEXT: -fsyntax-only  Run the preprocessor, parser and semantic analysis stages
 ! HELP-FC1-NEXT: -fxor-operator Enable .XOR. as a synonym of .NEQV.
 ! HELP-FC1-NEXT: -help  Display available options
Index: flang/test/Driver/driver-help-hidden.f90
===
--- flang/test/Driver/driver-help-hidden.f90
+++ flang/test/Driver/driver-help-hidden.f90
@@ -44,6 +44,7 @@
 ! CHECK-NEXT: -fno-integrated-as Disable the integrated assembler
 ! CHECK-NEXT: -fopenacc  Enable OpenACC
 ! CHECK-NEXT: -fopenmp   Parse OpenMP pragmas and generate parallel code.
+! CHECK-NEXT: -fpass-plugin= Load pass plugin from a dynamic shared object file (only with new pass manager).
 ! CHECK-NEXT: -fsyntax-only  Run the preprocessor, parser and semantic analysis stages
 ! CHECK-NEXT: -fxor-operator Enable .XOR. as a synonym 

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

[Iain Sandoe via Phabricator via cfe-commits]

iains added a comment.

FAOD: I think this should not be about "who's patches" - but about the 
compilation model and command lines that we want to end up with.

I am concerned that with the current scheme (with or without the change in this 
review) we could end up with three process launches per modular file:

- one to determine the deps
- one to produce the BMI
- one to produce the object

At least with GCC (and a patch series like the one I drafted) we can reduce 
that to 2

With the interfaces mentioned in P1184 , it is 
plausible to make that 1 (at the expense of potentially many stalled builds - 
but stalled early in compilation, so hopefully with the smallest memory 
footprint we could achieve).


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

https://reviews.llvm.org/D134267

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

[PATCH] D135608: Update implementation status of P2468R2

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 created this revision.
Herald added a project: All.
usaxena95 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135608

Files:
  clang/www/cxx_status.html


Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1432,7 +1432,7 @@
 
   The Equality Operator You Are Looking For
   https://wg21.link/P2468R2";>P2468R2
-  No
+  Clang 16
 
 
   De-deprecating volatile compound operations


Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1432,7 +1432,7 @@
 
   The Equality Operator You Are Looking For
   https://wg21.link/P2468R2";>P2468R2
-  No
+  Clang 16
 
 
   De-deprecating volatile compound operations
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D134529: [C++20][Clang] P2468R2 The Equality Operator You Are Looking For

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 added a comment.

Sent out https://reviews.llvm.org/D135608 to update implementation status.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134529

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

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

I thought this was settled quite a while ago and enshrined in the style guide: 
https://llvm.org/docs/CodingStandards.html#assert-liberally

`assert(0)` should not be used if something is reachable. We shouldn't have a 
"this violates an invariant, but if you don't have asserts enabled you do get 
some maybe-acceptable behavior".

I feel fairly strongly that any cases of "reachable asserts" should be changed 
to valid error handling or `llvm_report_error` and remaining `assert(0)` should 
be transformed to `llvm_unreachable`. (also, ideally, don't use 
branch-to-`unreachable` where possible, instead assert the condition - in cases 
where the `if` has side effects, sometimes that's the nicest way to write it, 
but might be clearer, if more verbose to use a local variable for the 
condition, then assert that the variable is true (and have the requisite 
"variable might be unused" cast))

> Historically, we've usually used llvm_unreachable for situations where we're 
> saying "this code cannot be reached; if it can, something else has gone 
> seriously wrong." For example, in code like: int foo(SomeEnum E) { switch (E) 
> { case One: return 1; default: return 2; } llvm_unreachable("getting here 
> would be mysterious"); } and we've used assert(0) for situations where we're 
> saying "this code is possible to reach only when there were mistakes 
> elsewhere which broke invariants we were relying on."

I don't think those are different things though - violating invariants is ~= 
something going seriously wrong.

(sorry, I guess I should debate this on the thread instead of here - but I 
think most of the folks on that thread did agree with the LLVM style guide/the 
direction here)

This, I think was also discussed about a decade ago in the LLVM community and 
resulted in r166821/2962d9599e463265edae599285bbc6351f1cc0ef which specifically 
"Suggests llvm_unreachable over assert(0)" and is the policy of LLVM - this 
change is consistent with that policy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135551

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

[PATCH] D135608: Update implementation status of P2468R2

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

Thanks for working on this




Comment at: clang/www/cxx_status.html:1435
   https://wg21.link/P2468R2";>P2468R2
-  No
+  Clang 16
 

Should be "unreleased"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135608

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

[PATCH] D135490: [clang/Sema] Follow-up for fix of non-deterministic order of `-Wunused-variable` diagnostic

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

I don't know too much about this. I guess have a DiagReceiver to force the 
emitting order is a good thing but it is kind of weird to have this just for 
unused warning.

I am guessing my suspect of removing decl from the scope is the cause of the 
slow down. Maybe we just force order on iteration?




Comment at: clang/include/clang/Sema/Scope.h:310
 
   decl_range decls() const {
 return decl_range(DeclsInScope.begin(), DeclsInScope.end());

I wonder if it would be easy if you just sort here and provide a consistent 
ordering when iterating?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135490

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

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Can't seem to find an llvm-dev/commits discussion for r166821, but I remember 
discussing it several times before, perhaps this one happened on IRC and so we 
may not have any record of it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135551

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

[PATCH] D135433: [clang][Interp] Implement while and do-while loops

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM!




Comment at: clang/lib/AST/Interp/ByteCodeStmtGen.cpp:293
+  LabelTy CondLabel = this->getLabel(); // Label before the condition.
+  LabelTy EndLabel = this->getLabel();  // Label after the loop
+  LoopScope LS(this, EndLabel, CondLabel);




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

https://reviews.llvm.org/D135433

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

[PATCH] D135490: [clang/Sema] Follow-up for fix of non-deterministic order of `-Wunused-variable` diagnostic

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

In D135490#3847454 , @steven_wu wrote:

> I don't know too much about this. I guess have a DiagReceiver to force the 
> emitting order is a good thing but it is kind of weird to have this just for 
> unused warning.
>
> I am guessing my suspect of removing decl from the scope is the cause of the 
> slow down. Maybe we just force order on iteration?

Ordering the decls every time `ActOnPopScope` is called is not cheap; ordering 
only the diagnostics (if any is emitted) is the most efficient way I could find.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135490

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

[PATCH] D135608: Update implementation status of P2468R2

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 466571.
usaxena95 marked an inline comment as done.
usaxena95 added a comment.

addressed comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135608

Files:
  clang/www/cxx_status.html


Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1432,7 +1432,7 @@
 
   The Equality Operator You Are Looking For
   https://wg21.link/P2468R2";>P2468R2
-  No
+  Clang 16
 
 
   De-deprecating volatile compound operations


Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1432,7 +1432,7 @@
 
   The Equality Operator You Are Looking For
   https://wg21.link/P2468R2";>P2468R2
-  No
+  Clang 16
 
 
   De-deprecating volatile compound operations
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135608: Update implementation status of P2468R2

[Corentin Jabot via Phabricator via cfe-commits]

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

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135608

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

[PATCH] D135569: [clang][Interp] Don't run functions immediately after compiling them

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

This function is testing whether something is potentially a constant 
expression. Something might not be a valid constant expression for two reasons: 
1) it uses some prohibited language construct, 2) it hit undefined behavior. 
You don't know if you hit undefined behavior until you run the function, so 
could that be why the function was being run? However, I don't know how you 
would run an arbitrary function that might accept arguments, so the `Run` call 
does look suspicious -- especially because it landed in the initial patch of 
the functionality (https://reviews.llvm.org/D64146) without comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135569

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

[clang] 6c49d5d - Fix unused variable warning from D134529

[Erich Keane via cfe-commits]

Author: Erich Keane
Date: 2022-10-10T11:42:43-07:00
New Revision: 6c49d5db30227d21e929bb12dc046c36ede67fad

URL: 
https://github.com/llvm/llvm-project/commit/6c49d5db30227d21e929bb12dc046c36ede67fad
DIFF: 
https://github.com/llvm/llvm-project/commit/6c49d5db30227d21e929bb12dc046c36ede67fad.diff

LOG: Fix unused variable warning from D134529

Added: 


Modified: 
clang/lib/Sema/SemaOverload.cpp

Removed: 




diff  --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index b3f98a0fe843e..c7e66d1e1d2ab 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -931,7 +931,7 @@ static bool shouldAddReversedEqEq(Sema &S, SourceLocation 
OpLoc,
   // target.
   DeclarationName NotEqOp = S.Context.DeclarationNames.getCXXOperatorName(
   OverloadedOperatorKind::OO_ExclaimEqual);
-  if (auto *MD = dyn_cast(EqFD)) {
+  if (isa(EqFD)) {
 // If F is a class member, search scope is class type of first operand.
 QualType RHS = FirstOperand->getType();
 auto *RHSRec = RHS->getAs();



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

[PATCH] D135545: [clang] Mention -Werror changes revived for Clang 16

[Aaron Ballman via Phabricator via cfe-commits]

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

Whoops, I thought I had already added those release notes, but I didn't! Thank 
you for catching that! LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135545

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

[PATCH] D135341: [clang] adds `__reference_constructs_from_temporary`

[Roy Jacobson via Phabricator via cfe-commits]

royjacobson added inline comments.



Comment at: clang/docs/LanguageExtensions.rst:1476
   is false. Note this trait will also return false when the initialization of
   ``T`` from ``U`` is ill-formed.
+* ``__reference_constructs_from_temporary(T, U)`` (C++)

We should deprecate this, as `__reference_constructs_from_temporary` is 
implemented now. There's a list of deprecated builtins in 
SemaExprCXX.cpp:DiagnoseBuiltinDeprecation so I think it should be pretty easy 
to add a warning for users that this is a non-standard attribute.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135341

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

[clang] 9d69c60 - Fix a typo in the Release Notes; NFC

[Aaron Ballman via cfe-commits]

Author: Aaron Ballman
Date: 2022-10-10T14:45:38-04:00
New Revision: 9d69c60357c8349774cbb592742593fb459c1a47

URL: 
https://github.com/llvm/llvm-project/commit/9d69c60357c8349774cbb592742593fb459c1a47
DIFF: 
https://github.com/llvm/llvm-project/commit/9d69c60357c8349774cbb592742593fb459c1a47.diff

LOG: Fix a typo in the Release Notes; NFC

Added: 


Modified: 
clang/docs/ReleaseNotes.rst

Removed: 




diff  --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 54f1afd06b28..bb32cad73f85 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -64,7 +64,7 @@ code bases.
 - ``-Wincompatible-function-pointer-types`` now defaults to an error in all C
   language modes. It may be downgraded to a warning with
   ``-Wno-error=incompatible-function-pointer-types`` or disabled entirely with
-  ``-Wno-implicit-function-pointer-types``.
+  ``-Wno-incompatible-function-pointer-types``.
 
   **NOTE:** We recommend that projects using configure scripts verify that the
   results do not change before/after setting



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

Re: [clang-tools-extra] 5d2d527 - [clangd] Avoid scanning up to end of file on each comment!

[David Blaikie via cfe-commits]

On Mon, Oct 10, 2022 at 11:13 AM Sam McCall  wrote:
>
> On Mon, 10 Oct 2022, 19:57 David Blaikie,  wrote:
>>
>> Could the underlying API be made more robust to handle StringRefs
>> rather than null terminated char* in the first place? (like
>> SourceManager::getCharacterData could return StringRef? Presumably at
>> some layer it already knows how long the file is - the underlying
>> MemoryBuffer, in this case)
>
> Maybe...
>
> You could return a StringRef(Pos, EOF) efficiently.
> However it's not a great fit for many of the existing (>150) callers:
>  - that are looking for the *endpoint* of some text

Yeah, I figured that was more what the API was about for better/worse.
I guess it's not worth having two entry points, a lower-level one that
returns StringRef and then the existing one that calls that and
returns the data pointer from the StringRef...

>  - which want to run the lexer (the returned char* is implicitly 
> null-terminated, StringRef would obscure that guarantee)

I guess by implication the lexer needs something null terminated? Be
nice if that wasn't the case too...

>  - the function is marked as "this is very hot", though I doubt it matters 
> that much

*nod* At least something to be careful about.

> It also leads to some unnatural code at callsites such as this, because "from 
> the start of this entity until the end of the file" is an unnatural range to 
> have, it would probably be easy to mistake for "from the start to the end of 
> this entity".

Yeah. :/

> I think what this function does is too low-level to make safe and obvious, 
> and unfortunately it's hard to build high-level things on top of it.

Yeah :/

> (e.g. StringRef getTextRange(SourceRange) would be great to have, but 
> actually you it needs to run the lexer and assert nontrivial invariants due 
> to design decisions elsewhere in clang)
>
>>
>> On Thu, Oct 6, 2022 at 2:39 AM Sam McCall via cfe-commits
>>  wrote:
>> >
>> >
>> > Author: Sam McCall
>> > Date: 2022-10-06T11:38:55+02:00
>> > New Revision: 5d2d527c32da2081b814ef8b446bc3e037f74b0a
>> >
>> > URL: 
>> > https://github.com/llvm/llvm-project/commit/5d2d527c32da2081b814ef8b446bc3e037f74b0a
>> > DIFF: 
>> > https://github.com/llvm/llvm-project/commit/5d2d527c32da2081b814ef8b446bc3e037f74b0a.diff
>> >
>> > LOG: [clangd] Avoid scanning up to end of file on each comment!
>> >
>> > Assigning char* (pointing at comment start) to StringRef was causing us
>> > to scan the rest of the source file looking for the null terminator.
>> >
>> > This seems to be eating about 8% of our *total* CPU!
>> >
>> > While fixing this, factor out the common bits from the two places we're
>> > parsing IWYU pragmas.
>> >
>> > Differential Revision: https://reviews.llvm.org/D135314
>> >
>> > Added:
>> >
>> >
>> > Modified:
>> > clang-tools-extra/clangd/Headers.cpp
>> > clang-tools-extra/clangd/Headers.h
>> > clang-tools-extra/clangd/index/CanonicalIncludes.cpp
>> > clang-tools-extra/clangd/unittests/HeadersTests.cpp
>> >
>> > Removed:
>> >
>> >
>> >
>> > 
>> > diff  --git a/clang-tools-extra/clangd/Headers.cpp 
>> > b/clang-tools-extra/clangd/Headers.cpp
>> > index 5231a47487bc7..52b954e921620 100644
>> > --- a/clang-tools-extra/clangd/Headers.cpp
>> > +++ b/clang-tools-extra/clangd/Headers.cpp
>> > @@ -22,9 +22,17 @@
>> >  namespace clang {
>> >  namespace clangd {
>> >
>> > -const char IWYUPragmaKeep[] = "// IWYU pragma: keep";
>> > -const char IWYUPragmaExport[] = "// IWYU pragma: export";
>> > -const char IWYUPragmaBeginExports[] = "// IWYU pragma: begin_exports";
>> > +llvm::Optional parseIWYUPragma(const char *Text) {
>> > +  // This gets called for every comment seen in the preamble, so it's 
>> > quite hot.
>> > +  constexpr llvm::StringLiteral IWYUPragma = "// IWYU pragma: ";
>> > +  if (strncmp(Text, IWYUPragma.data(), IWYUPragma.size()))
>> > +return llvm::None;
>> > +  Text += IWYUPragma.size();
>> > +  const char *End = Text;
>> > +  while (*End != 0 && *End != '\n')
>> > +++End;
>> > +  return StringRef(Text, End - Text);
>> > +}
>> >
>> >  class IncludeStructure::RecordHeaders : public PPCallbacks,
>> >  public CommentHandler {
>> > @@ -129,10 +137,10 @@ class IncludeStructure::RecordHeaders : public 
>> > PPCallbacks,
>> >}
>> >
>> >bool HandleComment(Preprocessor &PP, SourceRange Range) override {
>> > -bool Err = false;
>> > -llvm::StringRef Text = SM.getCharacterData(Range.getBegin(), &Err);
>> > -if (Err)
>> > +auto Pragma = parseIWYUPragma(SM.getCharacterData(Range.getBegin()));
>> > +if (!Pragma)
>> >return false;
>> > +
>> >  if (inMainFile()) {
>> >// Given:
>> >//
>> > @@ -150,8 +158,7 @@ class IncludeStructure::RecordHeaders : public 
>> > PPCallbacks,
>> >// will know that the next inclusion is behind the IWYU pragma.
>> >// FIXME: Support "IWYU pragma:

[clang] e66cfb6 - Update implementation status of P2468R2

[Utkarsh Saxena via cfe-commits]

Author: Utkarsh Saxena
Date: 2022-10-10T20:56:42+02:00
New Revision: e66cfb63ceed769f6ba61c733b9137fdc6acb1e0

URL: 
https://github.com/llvm/llvm-project/commit/e66cfb63ceed769f6ba61c733b9137fdc6acb1e0
DIFF: 
https://github.com/llvm/llvm-project/commit/e66cfb63ceed769f6ba61c733b9137fdc6acb1e0.diff

LOG: Update implementation status of P2468R2

Differential Revision: https://reviews.llvm.org/D135608

Added: 


Modified: 
clang/www/cxx_status.html

Removed: 




diff  --git a/clang/www/cxx_status.html b/clang/www/cxx_status.html
index 2d57897c7e8dc..632590a131a8b 100755
--- a/clang/www/cxx_status.html
+++ b/clang/www/cxx_status.html
@@ -1432,7 +1432,7 @@ C++2b implementation status
 
   The Equality Operator You Are Looking For
   https://wg21.link/P2468R2";>P2468R2
-  No
+  Clang 16
 
 
   De-deprecating volatile compound operations



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

[PATCH] D135608: Update implementation status of P2468R2

[Utkarsh Saxena 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 rGe66cfb63ceed: Update implementation status of P2468R2 
(authored by usaxena95).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135608

Files:
  clang/www/cxx_status.html


Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1432,7 +1432,7 @@
 
   The Equality Operator You Are Looking For
   https://wg21.link/P2468R2";>P2468R2
-  No
+  Clang 16
 
 
   De-deprecating volatile compound operations


Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1432,7 +1432,7 @@
 
   The Equality Operator You Are Looking For
   https://wg21.link/P2468R2";>P2468R2
-  No
+  Clang 16
 
 
   De-deprecating volatile compound operations
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129156: Add -fpass-plugin option to Flang

[Tarun Prabhu via Phabricator via cfe-commits]

tarunprabhu added a comment.

I tried an out-of-tree flang build. check-flang passes in that case too.


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

https://reviews.llvm.org/D129156

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

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D135551#3847444 , @dblaikie wrote:

> I thought this was settled quite a while ago and enshrined in the style 
> guide: https://llvm.org/docs/CodingStandards.html#assert-liberally
>
> `assert(0)` should not be used if something is reachable. We shouldn't have a 
> "this violates an invariant, but if you don't have asserts enabled you do get 
> some maybe-acceptable behavior".
>
> I feel fairly strongly that any cases of "reachable asserts" should be 
> changed to valid error handling or `llvm_report_error` and remaining 
> `assert(0)` should be transformed to `llvm_unreachable`. (also, ideally, 
> don't use branch-to-`unreachable` where possible, instead assert the 
> condition - in cases where the `if` has side effects, sometimes that's the 
> nicest way to write it, but might be clearer, if more verbose to use a local 
> variable for the condition, then assert that the variable is true (and have 
> the requisite "variable might be unused" cast))

I would be okay with that, but that's not what this patch was doing -- it was 
changing `assert(0)` into an `llvm_unreachable` more mechanically, and I don't 
think that's a valid transformation. The key, to me, is not losing the 
distinction between "reaching here is a programming mistake that you'd make 
during active development" vs "we never expect to reach this patch and want to 
optimize accordingly." `__builtin_unreachable` changes the debugging landscape 
far too much for me to want to see folks using it for "reaching here is a 
programming mistake" situations, *especially* in RelWithDebInfo builds where 
optimizations are enabled and may result in surprising call stacks and time 
travel debugging.

>> Historically, we've usually used llvm_unreachable for situations where we're 
>> saying "this code cannot be reached; if it can, something else has gone 
>> seriously wrong." For example, in code like: int foo(SomeEnum E) { switch 
>> (E) { case One: return 1; default: return 2; } llvm_unreachable("getting 
>> here would be mysterious"); } and we've used assert(0) for situations where 
>> we're saying "this code is possible to reach only when there were mistakes 
>> elsewhere which broke invariants we were relying on."
>
> I don't think those are different things though - violating invariants is ~= 
> something going seriously wrong.
>
> (sorry, I guess I should debate this on the thread instead of here - but I 
> think most of the folks on that thread did agree with the LLVM style 
> guide/the direction here)
>
> This, I think was also discussed about a decade ago in the LLVM community and 
> resulted in r166821/2962d9599e463265edae599285bbc6351f1cc0ef which 
> specifically "Suggests llvm_unreachable over assert(0)" and is the policy of 
> LLVM - this change is consistent with that policy.

I don't have the context for those changes in my email either, but regardless 
of what we thought ten years ago, we have code in the code base today that 
assumes a difference in severity between kinds of unreachable statements so we 
need to be careful when correcting mistakes. I think we're still in agreement 
that `llvm_unreachable` should be preferred over `assert(0)` in situations 
where the code is expected to be impossible to reach. I think we're also still 
in agreement that "correct error reporting" is preferred over `assert(0)`. 
Where we might still have daylight are the occasional situations where 
`assert(0)` gives a better experience -- when the code is possible to reach but 
reaching it signifies a developer (not user) mistake that is plausible to make 
when doing new development, such as when misusing the C interface (where there 
isn't always an error that should be reported via the API). I think these uses 
should generally be rare, but I don't think it's a "never do this" kind of 
situation.

`llvm_unreachable` asserts in debug mode, so it has the nice property we need 
when doing new development. But it doesn't convey the distinction in semantics. 
Maybe another approach is: `#define developer_bugcheck(msg) 
llvm_unreachable(msg)` (or something along those lines). We still get the 
assertion in debug mode, we then get the better optimization in release mode, 
but we don't lose the semantic information in the code base. It doesn't really 
help the RelWithDebInfo case though (where call stacks may be unrecognizable as 
a result of time travel optimizations) but maybe that's a tradeoff worth making?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135551

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

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

I would think we could convert every assert(0) to either 
`llvm::report_fatal_error` (guaranteed trap) or `llvm_unreachable()` (trap or 
optimize, depending on CMAKE configuration). The C API usage checks seem like 
good candidates for the former.

Also, not sure if everyone noticed, but the latter can now be configured to 
always trap by turning off the “optimize” CMAKE flag. This seems useful for 
fuzzing situations where you may not want asserts builds.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135551

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

[PATCH] D135545: [clang] Mention -Werror changes revived for Clang 16

[Michał Górny via Phabricator via cfe-commits]

mgorny accepted this revision.
mgorny added a comment.

Thanks for doing this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135545

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

[PATCH] D135500: [Clang] reject bit-fields as instruction operands in Microsoft style inline asm blocks.

[Tom Honermann via Phabricator via cfe-commits]

tahonermann updated this revision to Diff 466589.
tahonermann added a comment.

Rebased and addressed code review feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135500

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticCommonKinds.td
  clang/lib/Sema/SemaStmtAsm.cpp
  clang/test/Parser/ms-inline-asm.c

Index: clang/test/Parser/ms-inline-asm.c
===
--- clang/test/Parser/ms-inline-asm.c
+++ clang/test/Parser/ms-inline-asm.c
@@ -62,6 +62,18 @@
   __asm mov eax, offset A // expected-error {{offset operator cannot yet handle constants}}
 }
 
+// GH57791
+typedef struct S {
+  unsigned bf1:1; // expected-note {{bit-field is declared here}}
+  unsigned bf2:1; // expected-note {{bit-field is declared here}}
+} S;
+void t15(S s) {
+  __asm {
+mov eax, s.bf1 // expected-error {{an inline asm block cannot have an operand which is a bit-field}}
+mov s.bf2, eax // expected-error {{an inline asm block cannot have an operand which is a bit-field}}
+  }
+}
+
 int t_fail(void) { // expected-note {{to match this}}
   __asm 
   __asm { // expected-error 3 {{expected}} expected-note {{to match this}}
Index: clang/lib/Sema/SemaStmtAsm.cpp
===
--- clang/lib/Sema/SemaStmtAsm.cpp
+++ clang/lib/Sema/SemaStmtAsm.cpp
@@ -932,13 +932,24 @@
   bool IsSimple = (NumOutputs != 0 || NumInputs != 0);
   setFunctionHasBranchProtectedScope();
 
+  bool InvalidOperand = false;
   for (uint64_t I = 0; I < NumOutputs + NumInputs; ++I) {
-if (Exprs[I]->getType()->isBitIntType())
-  return StmtError(
-  Diag(Exprs[I]->getBeginLoc(), diag::err_asm_invalid_type)
-  << Exprs[I]->getType() << (I < NumOutputs)
-  << Exprs[I]->getSourceRange());
+Expr *E = Exprs[I];
+if (E->getType()->isBitIntType()) {
+  InvalidOperand = true;
+  Diag(E->getBeginLoc(), diag::err_asm_invalid_type)
+  << E->getType() << (I < NumOutputs)
+  << E->getSourceRange();
+} else if (E->refersToBitField()) {
+  InvalidOperand = true;
+  FieldDecl *BitField = E->getSourceBitField();
+  Diag(E->getBeginLoc(), diag::err_ms_asm_bitfield_unsupported)
+  << E->getSourceRange();
+  Diag(BitField->getLocation(), diag::note_bitfield_decl);
+}
   }
+  if (InvalidOperand)
+return StmtError();
 
   MSAsmStmt *NS =
 new (Context) MSAsmStmt(Context, AsmLoc, LBraceLoc, IsSimple,
Index: clang/include/clang/Basic/DiagnosticCommonKinds.td
===
--- clang/include/clang/Basic/DiagnosticCommonKinds.td
+++ clang/include/clang/Basic/DiagnosticCommonKinds.td
@@ -279,6 +279,9 @@
   def err_asm_invalid_type : Error<
 "invalid type %0 in asm %select{input|output}1">;
 
+  def err_ms_asm_bitfield_unsupported : Error<
+"an inline asm block cannot have an operand which is a bit-field">;
+
   def warn_stack_clash_protection_inline_asm : Warning<
 "Unable to protect inline asm that clobbers stack pointer against stack clash">,
 InGroup>;
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -129,6 +129,25 @@
   *p; // Now diagnosed as a warning-as-error.
 }
 
+- Clang now diagnoses use of a bit-field as an instruction operand in Microsoft
+  style inline asm blocks as an error. Previously, a bit-field operand yielded
+  the address of the allocation unit the bit-field was stored within; reads or
+  writes therefore had the potential to read or write nearby bit-fields. This
+  change fixes `issue 57791 `_.
+
+  .. code-block:: c++
+
+typedef struct S {
+  unsigned bf:1;
+} S;
+void f(S s) {
+  __asm {
+mov eax, s.bf // Now diagnosed as an error.
+mov s.bf, eax // Now diagnosed as an error.
+  }
+}
+
+
 What's New in Clang |release|?
 ==
 Some of the major new features and improvements to Clang are listed
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135370: Narrow inline namespace filtration for unqualified friend declarations

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.



> LGTM! Should we also add a release note or do we think this isn't enough of a 
> compile-time performance improvement to warrant that?

Good point @aaron.ballman, given we have only observed in a few corner case 
large TUs I'd say it's probably not that normally perceivable. Do you agree 
@troyj ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135370

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

[clang] 2bb86b6 - [clang] Mention -Werror changes revived for Clang 16

[Michał Górny via cfe-commits]

Author: Sam James
Date: 2022-10-10T21:44:30+02:00
New Revision: 2bb86b67447661077e30674eab7b8a27f8234b3f

URL: 
https://github.com/llvm/llvm-project/commit/2bb86b67447661077e30674eab7b8a27f8234b3f
DIFF: 
https://github.com/llvm/llvm-project/commit/2bb86b67447661077e30674eab7b8a27f8234b3f.diff

LOG: [clang] Mention -Werror changes revived for Clang 16

-Wimplicit-function-declaration and -Wimplicit-int become errors
by default in Clang 16 (originally in 15, but we reverted it in 15.0.1).

Mention it in the release notes like we did originally for Clang 15.

See 
https://discourse.llvm.org/t/configure-script-breakage-with-the-new-werror-implicit-function-declaration/65213
 for more context.

Signed-off-by: Sam James 

Differential Revision: https://reviews.llvm.org/D135545

Added: 


Modified: 
clang/docs/ReleaseNotes.rst

Removed: 




diff  --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index bb32cad73f85..29550f5376a7 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -61,6 +61,17 @@ code bases.
   into an error-only diagnostic in the next Clang release. Fixes
   `Issue 50055 `_.
 
+- The ``-Wimplicit-function-declaration`` and ``-Wimplicit-int`` warnings
+  now default to an error in C99, C11, and C17. As of C2x,
+  support for implicit function declarations and implicit int has been removed,
+  and the warning options will have no effect. Specifying ``-Wimplicit-int`` in
+  C89 mode will now issue warnings instead of being a noop.
+
+  **NOTE**: We recommend that projects using configure scripts verify that the
+  results do not change before/after setting
+  ``-Werror=implicit-function-declarations`` or ``-Wimplicit-int`` to avoid
+  incompatibility with Clang 16.
+
 - ``-Wincompatible-function-pointer-types`` now defaults to an error in all C
   language modes. It may be downgraded to a warning with
   ``-Wno-error=incompatible-function-pointer-types`` or disabled entirely with



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

[PATCH] D135545: [clang] Mention -Werror changes revived for Clang 16

[Michał Górny via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2bb86b674476: [clang] Mention -Werror changes revived for 
Clang 16 (authored by thesamesam, committed by mgorny).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135545

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -61,6 +61,17 @@
   into an error-only diagnostic in the next Clang release. Fixes
   `Issue 50055 `_.
 
+- The ``-Wimplicit-function-declaration`` and ``-Wimplicit-int`` warnings
+  now default to an error in C99, C11, and C17. As of C2x,
+  support for implicit function declarations and implicit int has been removed,
+  and the warning options will have no effect. Specifying ``-Wimplicit-int`` in
+  C89 mode will now issue warnings instead of being a noop.
+
+  **NOTE**: We recommend that projects using configure scripts verify that the
+  results do not change before/after setting
+  ``-Werror=implicit-function-declarations`` or ``-Wimplicit-int`` to avoid
+  incompatibility with Clang 16.
+
 - ``-Wincompatible-function-pointer-types`` now defaults to an error in all C
   language modes. It may be downgraded to a warning with
   ``-Wno-error=incompatible-function-pointer-types`` or disabled entirely with


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -61,6 +61,17 @@
   into an error-only diagnostic in the next Clang release. Fixes
   `Issue 50055 `_.
 
+- The ``-Wimplicit-function-declaration`` and ``-Wimplicit-int`` warnings
+  now default to an error in C99, C11, and C17. As of C2x,
+  support for implicit function declarations and implicit int has been removed,
+  and the warning options will have no effect. Specifying ``-Wimplicit-int`` in
+  C89 mode will now issue warnings instead of being a noop.
+
+  **NOTE**: We recommend that projects using configure scripts verify that the
+  results do not change before/after setting
+  ``-Werror=implicit-function-declarations`` or ``-Wimplicit-int`` to avoid
+  incompatibility with Clang 16.
+
 - ``-Wincompatible-function-pointer-types`` now defaults to an error in all C
   language modes. It may be downgraded to a warning with
   ``-Wno-error=incompatible-function-pointer-types`` or disabled entirely with
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits