[PATCH] D142836: [clangd] Add symbol mappings for `std::experimental::filesystem`

[Younan Zhang via Phabricator via cfe-commits]

zyounan created this revision.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
zyounan edited the summary of this revision.
zyounan added reviewers: hokein, sammccall, nridge.
zyounan published this revision for review.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added projects: clang, clang-tools-extra.

Clangd maintains a symbol map from standard library, in order to prevent
unexpected header/symbol leaks from internal files. (e.g. files under
`bits/` for libstdc++) This symbol map was generated by a python script
that parses pages of offline cppreference archive. The script didn't
handle the case for `std::experimental::`, where most symbols are from
TS. It works well as symbols are directly laid out in the corresponding
header under `experimental` directory for most of time.

However, libstdc++'s implementation split symbols of TS FS into a few
header files located in `experimental/bits`. This would make the code
completion provide internal headers when we simply select the symbols.

This fixes https://github.com/clangd/clangd/issues/1481


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142836

Files:
  clang-tools-extra/clangd/index/CanonicalIncludes.cpp
  clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
  clang/include/clang/Tooling/Inclusions/TsStdSymbolMap.inc

Index: clang/include/clang/Tooling/Inclusions/TsStdSymbolMap.inc
===
--- /dev/null
+++ clang/include/clang/Tooling/Inclusions/TsStdSymbolMap.inc
@@ -0,0 +1,55 @@
+// FIXME: Add a parser for experimental pages to `clang/tools/include-mapping/cppreference_parser.py`
+
+SYMBOL(absolute, std::experimental::filesystem::, )
+SYMBOL(canonical, std::experimental::filesystem::, )
+SYMBOL(copy, std::experimental::filesystem::, )
+SYMBOL(copy_file, std::experimental::filesystem::, )
+SYMBOL(copy_options, std::experimental::filesystem::, )
+SYMBOL(copy_symlink, std::experimental::filesystem::, )
+SYMBOL(create_directories, std::experimental::filesystem::, )
+SYMBOL(create_directory, std::experimental::filesystem::, )
+SYMBOL(create_directory_symlink, std::experimental::filesystem::, )
+SYMBOL(create_hard_link, std::experimental::filesystem::, )
+SYMBOL(create_symlink, std::experimental::filesystem::, )
+SYMBOL(current_path, std::experimental::filesystem::, )
+SYMBOL(directory_entry, std::experimental::filesystem::, )
+SYMBOL(directory_iterator, std::experimental::filesystem::, )
+SYMBOL(directory_options, std::experimental::filesystem::, )
+SYMBOL(equivalent, std::experimental::filesystem::, )
+SYMBOL(exists, std::experimental::filesystem::, )
+SYMBOL(file_size, std::experimental::filesystem::, )
+SYMBOL(file_status, std::experimental::filesystem::, )
+SYMBOL(file_time_type, std::experimental::filesystem::, )
+SYMBOL(file_type, std::experimental::filesystem::, )
+SYMBOL(filesystem_error, std::experimental::filesystem::, )
+SYMBOL(hard_link_count, std::experimental::filesystem::, )
+SYMBOL(is_block_file, std::experimental::filesystem::, )
+SYMBOL(is_character_file, std::experimental::filesystem::, )
+SYMBOL(is_directory, std::experimental::filesystem::, )
+SYMBOL(is_empty, std::experimental::filesystem::, )
+SYMBOL(is_fifo, std::experimental::filesystem::, )
+SYMBOL(is_other, std::experimental::filesystem::, )
+SYMBOL(is_regular_file, std::experimental::filesystem::, )
+SYMBOL(is_socket, std::experimental::filesystem::, )
+SYMBOL(is_symlink, std::experimental::filesystem::, )
+SYMBOL(last_write_time, std::experimental::filesystem::, )
+SYMBOL(path, std::experimental::filesystem::, )
+SYMBOL(perm_options, std::experimental::filesystem::, )
+SYMBOL(permissions, std::experimental::filesystem::, )
+SYMBOL(perms, std::experimental::filesystem::, )
+SYMBOL(proximate, std::experimental::filesystem::, )
+SYMBOL(read_symlink, std::experimental::filesystem::, )
+SYMBOL(recursive_directory_iterator, std::experimental::filesystem::, )
+SYMBOL(relative, std::experimental::filesystem::, )
+SYMBOL(remove, std::experimental::filesystem::, )
+SYMBOL(remove_all, std::experimental::filesystem::, )
+SYMBOL(rename, std::experimental::filesystem::, )
+SYMBOL(resize_file, std::experimental::filesystem::, )
+SYMBOL(space, std::experimental::filesystem::, )
+SYMBOL(space_info, std::experimental::filesystem::, )
+SYMBOL(status, std::experimental::filesystem::, )
+SYMBOL(status_known, std::experimental::filesystem::, )
+SYMBOL(symlink_status, std::experimental::filesystem::, )
+SYMBOL(temp_directory_path, std::experimental::filesystem::, )
+SYMBOL(u8path, std::experimental::filesystem::, )
+SYMBOL(weakly_canonical, std::experimental::filesystem::, )
Index: clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
===
--- clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
+++ clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
@@ -62,6 +62,23 @@
   

[PATCH] D142836: [clangd] Add symbol mappings for `std::experimental::filesystem`

[Younan Zhang via Phabricator via cfe-commits]

zyounan updated this revision to Diff 493081.
zyounan added a comment.

Do not format inc file


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142836

Files:
  clang-tools-extra/clangd/index/CanonicalIncludes.cpp
  clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
  clang/include/clang/Tooling/Inclusions/TsStdSymbolMap.inc

Index: clang/include/clang/Tooling/Inclusions/TsStdSymbolMap.inc
===
--- /dev/null
+++ clang/include/clang/Tooling/Inclusions/TsStdSymbolMap.inc
@@ -0,0 +1,57 @@
+// FIXME: Add a parser for experimental pages to `clang/tools/include-mapping/cppreference_parser.py`
+
+// clang-format off
+SYMBOL(absolute, std::experimental::filesystem::, )
+SYMBOL(canonical, std::experimental::filesystem::, )
+SYMBOL(copy, std::experimental::filesystem::, )
+SYMBOL(copy_file, std::experimental::filesystem::, )
+SYMBOL(copy_options, std::experimental::filesystem::, )
+SYMBOL(copy_symlink, std::experimental::filesystem::, )
+SYMBOL(create_directories, std::experimental::filesystem::, )
+SYMBOL(create_directory, std::experimental::filesystem::, )
+SYMBOL(create_directory_symlink, std::experimental::filesystem::, )
+SYMBOL(create_hard_link, std::experimental::filesystem::, )
+SYMBOL(create_symlink, std::experimental::filesystem::, )
+SYMBOL(current_path, std::experimental::filesystem::, )
+SYMBOL(directory_entry, std::experimental::filesystem::, )
+SYMBOL(directory_iterator, std::experimental::filesystem::, )
+SYMBOL(directory_options, std::experimental::filesystem::, )
+SYMBOL(equivalent, std::experimental::filesystem::, )
+SYMBOL(exists, std::experimental::filesystem::, )
+SYMBOL(file_size, std::experimental::filesystem::, )
+SYMBOL(file_status, std::experimental::filesystem::, )
+SYMBOL(file_time_type, std::experimental::filesystem::, )
+SYMBOL(file_type, std::experimental::filesystem::, )
+SYMBOL(filesystem_error, std::experimental::filesystem::, )
+SYMBOL(hard_link_count, std::experimental::filesystem::, )
+SYMBOL(is_block_file, std::experimental::filesystem::, )
+SYMBOL(is_character_file, std::experimental::filesystem::, )
+SYMBOL(is_directory, std::experimental::filesystem::, )
+SYMBOL(is_empty, std::experimental::filesystem::, )
+SYMBOL(is_fifo, std::experimental::filesystem::, )
+SYMBOL(is_other, std::experimental::filesystem::, )
+SYMBOL(is_regular_file, std::experimental::filesystem::, )
+SYMBOL(is_socket, std::experimental::filesystem::, )
+SYMBOL(is_symlink, std::experimental::filesystem::, )
+SYMBOL(last_write_time, std::experimental::filesystem::, )
+SYMBOL(path, std::experimental::filesystem::, )
+SYMBOL(perm_options, std::experimental::filesystem::, )
+SYMBOL(permissions, std::experimental::filesystem::, )
+SYMBOL(perms, std::experimental::filesystem::, )
+SYMBOL(proximate, std::experimental::filesystem::, )
+SYMBOL(read_symlink, std::experimental::filesystem::, )
+SYMBOL(recursive_directory_iterator, std::experimental::filesystem::, )
+SYMBOL(relative, std::experimental::filesystem::, )
+SYMBOL(remove, std::experimental::filesystem::, )
+SYMBOL(remove_all, std::experimental::filesystem::, )
+SYMBOL(rename, std::experimental::filesystem::, )
+SYMBOL(resize_file, std::experimental::filesystem::, )
+SYMBOL(space, std::experimental::filesystem::, )
+SYMBOL(space_info, std::experimental::filesystem::, )
+SYMBOL(status, std::experimental::filesystem::, )
+SYMBOL(status_known, std::experimental::filesystem::, )
+SYMBOL(symlink_status, std::experimental::filesystem::, )
+SYMBOL(temp_directory_path, std::experimental::filesystem::, )
+SYMBOL(u8path, std::experimental::filesystem::, )
+SYMBOL(weakly_canonical, std::experimental::filesystem::, )
+// clang-format on
Index: clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
===
--- clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
+++ clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
@@ -62,6 +62,23 @@
   EXPECT_EQ("", CI.mapHeader(File));
 }
 
+TEST(CanonicalIncludesTest, CXXSymbolsFromTS) {
+  CanonicalIncludes CI;
+  auto Language = LangOptions();
+  Language.CPlusPlus = true;
+  CI.addSystemHeadersMapping(Language);
+
+  EXPECT_EQ("",
+CI.mapSymbol("std::experimental::filesystem::path"));
+  EXPECT_EQ("",
+CI.mapSymbol("std::experimental::filesystem::file_type"));
+  EXPECT_EQ("",
+CI.mapSymbol("std::experimental::filesystem::file_status"));
+  EXPECT_EQ("",
+CI.mapSymbol(
+"std::experimental::filesystem::recursive_directory_iterator"));
+}
+
 TEST(CanonicalIncludesTest, PathMapping) {
   auto InMemFS = llvm::makeIntrusiveRefCnt();
   FileManager Files(FileSystemOptions(), InMemFS);
Index: clang-tools-extra/clangd/index/CanonicalIncludes.cpp
===
--- clang-tools-extra/

[PATCH] D142578: [Clang][Doc] Edit the Clang release notes

[Roy Jacobson via Phabricator via cfe-commits]

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

In D142578#4088011 , @tschuett wrote:

> IDK. Clang 16 is fully is a fully conformant C++20 except for some DRs. Or 
> beginning with Clang 16, we start a long-term project to overhaul the 
> diagnostics.
>
> I agree that breaking changes are important.

I contemplated on this for a bit, and I don't have anything that I feel is 
representative enough and fits into one paragraph. I'm still not against the 
general idea though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142578

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

[PATCH] D142014: [clangd] fix wrong CalleeArgInfo in the hover

[Vincent Hong via Phabricator via cfe-commits]

v1nh1shungry added inline comments.



Comment at: clang-tools-extra/clangd/Hover.cpp:994
   HI.CalleeArgInfo.emplace(toHoverInfoParam(PVD, PP));
+  PassType.PassBy = getPassMode(PVD->getType());
+}

kadircet wrote:
> v1nh1shungry wrote:
> > kadircet wrote:
> > > v1nh1shungry wrote:
> > > > kadircet wrote:
> > > > > sorry for showing up late to the party but instead of changing rest 
> > > > > of the code, can we apply this logic only when there are no implicit 
> > > > > casts/conversions between the arg and callexpr (i.e `N == 
> > > > > &OuterNode`)?
> > > > To make sure I understand it correctly, do you mean I should give up 
> > > > any other code changes I made but keep this logic, and put this logic 
> > > > into the `N == &OuterNode` branch?
> > > > 
> > > > Sorry if I misunderstood anything! Shame for not being a good reader :(
> > > > To make sure I understand it correctly, do you mean I should give up 
> > > > any other code changes I made but keep this logic, and put this logic 
> > > > into the N == &OuterNode branch?
> > > 
> > > Somewhat.
> > > 
> > > Basically this code had the assumption that if we don't see any 
> > > casts/conversions between the expression creating the argument and the 
> > > expression getting passed to the callexpr, it must be passed by 
> > > reference, and this was wrong. Even before the patch that added handling 
> > > for literals.
> > > 
> > > The rest of the handling for casts/conversions/constructors in between 
> > > have been working so far and was constructed based on what each 
> > > particular cast does, not for specific cases hence they're easier (for 
> > > the lack of a better word) to reason about. Hence I'd rather keep them as 
> > > is, especially the changes in handling `MaterializeTemporaryExpr` don't 
> > > sound right. I can see the example you've at hand, but we shouldn't be 
> > > producing "converted" results for it anyway (the AST should have a NoOp 
> > > implicit cast to `const int` and then a `MaterializeTemporaryExpr`, which 
> > > shouldn't generated any converted signals with the existing code already).
> > > 
> > > Hence the my proposal is getting rid of the assumption around "if we 
> > > don't see any casts/conversions between the expression creating the 
> > > argument and the expression getting passed to the callexpr, it must be 
> > > passed by reference", and use the type information in `ParmVarDecl` 
> > > directly when we don't have any implicit nodes in between to infer 
> > > `PassBy`.
> > > This should imply also getting rid of the special case for literals (`if 
> > > (isLiteral(E) && N->Parent == OuterNode.Parent)`).
> > > 
> > > Does that make sense?
> > Thanks for the detailed explanation! But before we go ahead here, what do 
> > you think about the new test case I'm talking about above? Do you agree 
> > with my conclusion?
> i suppose you mean:
> 
> ```
> void foobar(const float &);
> int main() {
>   foobar(0);
>   ^
> }
> ```
> 
> first of all the version of the patch that i propose doesn't involve any 
> changes in behaviour here (as we actually have an implicit cast in between, 
> and i am suggesting finding out passby based on type of the parmvardecl only 
> when there are no casts in between).
> 
> i can see @nridge 's reasoning about indicating copies by saying pass by 
> value vs ref, which unfortunately doesn't align with C++ semantics directly 
> (as what we have here is a prvalue, and it is indeed passed by value, without 
> any copies to the callee).
> 
> it isn't very obvious anywhere but the main functionality we wanted to 
> provide to the developer was help them figure out if a function call can 
> mutate a parameter they were passing in, therefore it didn't prioritise 
> literals at all. we probably should've made better wording choices in the UI 
> and talked about "immutability". hence from functionality point of view 
> calling this pass by `value` vs `const ref` doesn't make a huge difference 
> (but that's probably only in my mind and people are already using it to infer 
> other things like whether we're going to trigger copies).
> 
> so i'd actually leave this case as-is, and think about what we're actually 
> trying to provide by showing arg info on literals. as it's currently trying 
> to overload the meaning of `passby` and causing confusions. since the initial 
> intent was to just convey "immutability" one suggestion would be to just hide 
> the `passby` information for literals.
> otherwise from value categories point of view, these are always passed by 
> value, but this is going to create confusion for people that are using it to 
> infer "copies" and getting that right, while preserving the semantics around 
> "is this mutable" just complicates things.
> 
> best thing moving forward would probably be to just have two separate fields, 
> one indicating mutability and another indicating copies and not talking about 
> pass by type a

[PATCH] D140915: [clangd] Fix getQueryScopes for using-directive with inline namespace

[Tom Praschan via Phabricator via cfe-commits]

tom-anders updated this revision to Diff 493096.
tom-anders marked 8 inline comments as done.
tom-anders added a comment.

Address review comments, add regression test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140915

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -465,6 +465,18 @@
   Not(Contains(AllOf(qualifier(""), named("foo");
 }
 
+// https://github.com/clangd/clangd/issues/1451
+TEST(CompletionTest, QualificationWithInlineNamespace) {
+  auto Results = completions(R"cpp(
+namespace a { inline namespace b {} }
+using namespace a::b;
+void f() { Foo^ }
+  )cpp",
+ {cls("a::Foo")});
+  EXPECT_THAT(Results.Completions,
+  UnorderedElementsAre(AllOf(qualifier("a::"), named("Foo";
+}
+
 TEST(CompletionTest, InjectedTypename) {
   // These are suppressed when accessed as a member...
   EXPECT_THAT(completions("struct X{}; void foo(){ X().^ }").Completions,
Index: clang-tools-extra/clangd/CodeComplete.cpp
===
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -307,7 +307,7 @@
 struct CodeCompletionBuilder {
   CodeCompletionBuilder(ASTContext *ASTCtx, const CompletionCandidate &C,
 CodeCompletionString *SemaCCS,
-llvm::ArrayRef QueryScopes,
+llvm::ArrayRef AccessibleScopes,
 const IncludeInserter &Includes,
 llvm::StringRef FileName,
 CodeCompletionContext::Kind ContextKind,
@@ -361,7 +361,7 @@
   // avoids unneeded qualifiers in cases like with `using ns::X`.
   if (Completion.RequiredQualifier.empty() && !C.SemaResult) {
 llvm::StringRef ShortestQualifier = C.IndexResult->Scope;
-for (llvm::StringRef Scope : QueryScopes) {
+for (llvm::StringRef Scope : AccessibleScopes) {
   llvm::StringRef Qualifier = C.IndexResult->Scope;
   if (Qualifier.consume_front(Scope) &&
   Qualifier.size() < ShortestQualifier.size())
@@ -638,21 +638,40 @@
   //
   // Examples of unqualified completion:
   //
-  //   "vec^"   => {""}
-  //   "using namespace std; vec^"  => {"", "std::"}
-  //   "using namespace std; namespace ns { vec^ }" => {"ns::", "std::", ""}
+  //   "vec^"=> {""}
+  //   "using namespace std; vec^"   => {"", "std::"}
+  //   "namespace ns {inline namespace ni { struct Foo {}}}
+  //using namespace ns::ni; Fo^ "=> {"", "ns::ni::"}
+  //   "using namespace std; namespace ns { vec^ }"  => {"ns::", "std::", ""}
   //
   // "" for global namespace, "ns::" for normal namespace.
   std::vector AccessibleScopes;
+  // This is an overestimate of AccessibleScopes, e.g. it ignores inline
+  // namespaces, to fetch more relevant symbols from index.
+  std::vector QueryScopes;
   // The full scope qualifier as typed by the user (without the leading "::").
   // Set if the qualifier is not fully resolved by Sema.
   llvm::Optional UnresolvedQualifier;
 
+  std::optional EnclosingNamespace;
+
+  bool AllowAllScopes = false;
+
+  // Scopes that are accessible from current context. Used for dropping
+  // unnecessary namespecifiers.
+  std::vector scopesForQualification() {
+std::set Results;
+for (llvm::StringRef AS : AccessibleScopes)
+  Results.insert(
+  (AS + (UnresolvedQualifier ? *UnresolvedQualifier : "")).str());
+return {Results.begin(), Results.end()};
+  }
+
   // Construct scopes being queried in indexes. The results are deduplicated.
   // This method format the scopes to match the index request representation.
   std::vector scopesForIndexQuery() {
 std::set Results;
-for (llvm::StringRef AS : AccessibleScopes)
+for (llvm::StringRef AS : QueryScopes)
   Results.insert(
   (AS + (UnresolvedQualifier ? *UnresolvedQualifier : "")).str());
 return {Results.begin(), Results.end()};
@@ -662,16 +681,19 @@
 // Get all scopes that will be queried in indexes and whether symbols from
 // any scope is allowed. The first scope in the list is the preferred scope
 // (e.g. enclosing namespace).
-std::pair, bool>
-getQueryScopes(CodeCompletionContext &CCContext, const Sema &CCSema,
-   const CompletionPrefix &HeuristicPrefix,
-   const CodeCompleteOptions &Opts) {
+SpecifiedScope getQueryScopes(CodeCompletionContext &CCContext,
+  

[PATCH] D140915: [clangd] Fix getQueryScopes for using-directive with inline namespace

[Tom Praschan via Phabricator via cfe-commits]

tom-anders marked an inline comment as done.
tom-anders added inline comments.



Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1714
+}
+llvm::copy_if(SpecifiedScopes.scopesForIndexQuery(),
+  std::back_inserter(QueryScopes),

Should I add a `reserve` here for QueryScopes and AccessibleScopes? Would make 
this a bit more complicated for a small performance boost.



Comment at: clang-tools-extra/clangd/CodeComplete.cpp:712
+std::vector EnclosingAtFrontForCompletion;
 std::string EnclosingScope = printNamespaceScope(*CCSema.CurContext);
+EnclosingAtFrontForIndex.push_back(EnclosingScope);

kadircet wrote:
> note that this is actually going to skip inline namespaces (and you're using 
> that in the returned set)
Hm I should probably fix this and add another regression test for this..?



Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1660
 // First scope is the (modified) enclosing scope.
 QueryScopes = Scopes.scopesForIndexQuery();
 ScopeProximity.emplace(QueryScopes);

kadircet wrote:
> we should be setting `AccessibleScopes` too
Ah thanks, that's what caused the one failing test. I just copied over 
QueryScopes here for now, looks like this doesn't need any special handling for 
inline namespaces, does it?



Comment at: clang-tools-extra/clangd/CodeComplete.cpp:674
+else if (const auto *ND = dyn_cast(Context)) {
+  if (ND->isInlineNamespace())
+Scopes.AccessibleScopes.push_back(printQualifiedName(*ND) + "::");

kadircet wrote:
> tom-anders wrote:
> > kadircet wrote:
> > > tom-anders wrote:
> > > > kadircet wrote:
> > > > > tom-anders wrote:
> > > > > > kadircet wrote:
> > > > > > > since we know that the `Context` is a `NamespaceDecl` it should 
> > > > > > > be safe to use `printQualifiedName` always. any reason for the 
> > > > > > > extra branching here (apart from minimizing the change to 
> > > > > > > behaviour)? if not I think we can get rid of the special casing.
> > > > > > Unfortunately, this fails CompletionTest.EnclosingScopeComesFirst 
> > > > > > and CompletionTest.NoDuplicatedQueryScopes, I think because of 
> > > > > > anonymous namespaces (where printNameSpaceScope would return an 
> > > > > > empty string, but (printQualifiedName(*ND) + "::" does not). 
> > > > > i see. taking a closer look at this `getQueryScopes` is used for two 
> > > > > things:
> > > > > - The scopes to query with fuzzyfind requests, hence this should use 
> > > > > the same "serialization" as symbolcollector (which only strips anon 
> > > > > namespaces today, but initially it were to strip both anon & inline 
> > > > > namespaces. it got changed inside clang without clangd tests catching 
> > > > > it).
> > > > > - The shortening of the fully qualified name in 
> > > > > `CodeCompletionBuilder`. Not having inline namespaces spelled in the 
> > > > > available namespaces implies getting wrong qualifiers (such as the 
> > > > > bug you're fixing).
> > > > > 
> > > > > so considering the requirements here:
> > > > > - when querying index, we actually want to hide inline namespaces (as 
> > > > > `ns::hidden::Foo` should be a viable alternative even if only `ns::` 
> > > > > is accessible). so we should actually fix `printQualifiedName` to set 
> > > > > `SuppressInlineNamespace` in printing policy to restore the old 
> > > > > behaviour (and keep using `printNamespaceScope` here).
> > > > > - inside `CodeCompletionBuilder`, we shouldn't use the same scopes we 
> > > > > use during index queries. we should use the visible namespaces while 
> > > > > preserving inline namespace information and only ignoring the 
> > > > > anonymous namespaces.
> > > > > 
> > > > > hence can we have 2 separate scopes in `CodeCompleteFlow` instead?
> > > > > One called `QueryScopes`, which has the behavior we have today 
> > > > > (fixing printQualifiedName is a separate issues).
> > > > > Other called `AccessibleScopes`, which has accessible namespaces 
> > > > > spelled **with** inline namespaces, so that we can get proper 
> > > > > qualification during code-complete.
> > > > > 
> > > > > does that make sense?
> > > > tbh I'm a bit confused - I understand your requirements, but am not 
> > > > sure I understand your proposed solution. Can you expand a bit further? 
> > > > Looking at the code, there are already both `QueryScopes` and 
> > > > `AccessibleScopes` variables/fields in various classes, I'm not really 
> > > > sure at which places you want to make changes.
> > > sorry for the long and confusing answer :D
> > > 
> > > I was talking about `CodeCompleteFlow` class specifically, inside 
> > > `CodeComplete.cpp`. Currently it only has `QueryScopes`, derived from the 
> > > visible contexts reported by Sema. Unfortunately it loses some 
> > > granularity to fetch more symbols from index hence it should not be used 
> > > when

[PATCH] D142739: Standalone checker for use of _Optional qualifier

[Christopher Bazley via Phabricator via cfe-commits]

chrisbazley added a comment.

Please refer to 
https://discourse.llvm.org/t/rfc-optional-a-type-qualifier-to-indicate-pointer-nullability/
 and https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3089.pdf for the wider 
context of this commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142739

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

[PATCH] D142741: Fix ProgramState::isNull for non-region symbols

[Christopher Bazley via Phabricator via cfe-commits]

chrisbazley added a comment.

Please refer to 
https://discourse.llvm.org/t/rfc-optional-a-type-qualifier-to-indicate-pointer-nullability/
 and https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3089.pdf for the wider 
context of this commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142741

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

[PATCH] D142742: Generate ImplicitNullDerefEvent from CallAndMessageChecker

[Christopher Bazley via Phabricator via cfe-commits]

chrisbazley added a comment.

Please refer to 
https://discourse.llvm.org/t/rfc-optional-a-type-qualifier-to-indicate-pointer-nullability/
 and https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3089.pdf for the wider 
context of this commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142742

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

[PATCH] D142743: Fix nullability checking of top-level functions

[Christopher Bazley via Phabricator via cfe-commits]

chrisbazley added a comment.

Please refer to 
https://discourse.llvm.org/t/rfc-optional-a-type-qualifier-to-indicate-pointer-nullability/
 and https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3089.pdf for the wider 
context of this commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142743

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

[PATCH] D142744: Re-analyze functions as top-level

[Christopher Bazley via Phabricator via cfe-commits]

chrisbazley added a comment.

Please refer to 
https://discourse.llvm.org/t/rfc-optional-a-type-qualifier-to-indicate-pointer-nullability/
 and https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3089.pdf for the wider 
context of this commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142744

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

[PATCH] D142733: Add _Optional as fast qualifier

[Christopher Bazley via Phabricator via cfe-commits]

chrisbazley added a comment.

Please refer to 
https://discourse.llvm.org/t/rfc-optional-a-type-qualifier-to-indicate-pointer-nullability/
 and https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3089.pdf for the wider 
context of this commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142733

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

[PATCH] D142734: Updated CheckAddressOfOperand

[Christopher Bazley via Phabricator via cfe-commits]

chrisbazley added a comment.

Please refer to 
https://discourse.llvm.org/t/rfc-optional-a-type-qualifier-to-indicate-pointer-nullability/
 and https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3089.pdf for the wider 
context of this commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142734

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

[PATCH] D142736: Add QualType::getNullability for _Optional

[Christopher Bazley via Phabricator via cfe-commits]

chrisbazley added a comment.

Please refer to 
https://discourse.llvm.org/t/rfc-optional-a-type-qualifier-to-indicate-pointer-nullability/
 and https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3089.pdf for the wider 
context of this commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142736

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

[PATCH] D142737: Updated getNullabilityAnnotation for checkers

[Christopher Bazley via Phabricator via cfe-commits]

chrisbazley added a comment.

Please refer to 
https://discourse.llvm.org/t/rfc-optional-a-type-qualifier-to-indicate-pointer-nullability/
 and https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3089.pdf for the wider 
context of this commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142737

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

[PATCH] D142738: Warn if _Optional used at top-level of decl

[Christopher Bazley via Phabricator via cfe-commits]

chrisbazley added a comment.

Please refer to 
https://discourse.llvm.org/t/rfc-optional-a-type-qualifier-to-indicate-pointer-nullability/
 and https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3089.pdf for the wider 
context of this commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142738

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

cfe-commits@lists.llvm.org

[David K Turner via Phabricator via cfe-commits]

dkt01 marked 11 inline comments as done.
dkt01 added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:1195-1198
+  // Handle unbalanced braces.
+  if (!Scopes.empty())
+Scopes.pop_back();
   // Lines can start with '}'.

owenpan wrote:
> I don't think it's about unbalanced braces here.
`if (!Scopes.empty())` handles unbalanced braces.  `if(Tok->Previous)` handles 
the case where a line starts with an rbrace.



Comment at: clang/lib/Format/TokenAnnotator.cpp:2481-2482
+if (Tok.is(tok::amp) && (PrevToken && PrevToken->Tok.isAnyIdentifier()) &&
+(!PrevToken->getPreviousNonComment() ||
+ IsChainedOperatorAmpOrMember(PrevToken->getPreviousNonComment())) &&
+(NextToken && NextToken->Tok.isAnyIdentifier()) &&

owenpan wrote:
> The lambda would check that `PrevToken` is nonnull.
`!PrevToken->getPreviousNonComment() ||` is a different check than the null 
check in the lambda.  It's acceptable to have no token two tokens prior because 
that indicates the ampersand is the second token of the line.


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

https://reviews.llvm.org/D141959

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

cfe-commits@lists.llvm.org

[David K Turner via Phabricator via cfe-commits]

dkt01 updated this revision to Diff 493104.
dkt01 added a comment.

Updates suggested in owenpan's review.


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

https://reviews.llvm.org/D141959

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/lib/Format/TokenAnnotator.h
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/TokenAnnotatorTest.cpp

Index: clang/unittests/Format/TokenAnnotatorTest.cpp
===
--- clang/unittests/Format/TokenAnnotatorTest.cpp
+++ clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -175,6 +175,73 @@
   ASSERT_EQ(Tokens.size(), 17u) << Tokens;
   EXPECT_TOKEN(Tokens[9], tok::ampamp, TT_PointerOrReference);
   EXPECT_TOKEN(Tokens[12], tok::ampamp, TT_PointerOrReference);
+
+  Tokens = annotate("Type1 &val1 = val2;");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::amp, TT_PointerOrReference);
+
+  Tokens = annotate("Type1 *val1 = &val2;");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::star, TT_PointerOrReference);
+  EXPECT_TOKEN(Tokens[4], tok::amp, TT_UnaryOperator);
+
+  Tokens = annotate("val1 & val2;");
+  ASSERT_EQ(Tokens.size(), 5u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::amp, TT_BinaryOperator);
+
+  Tokens = annotate("val1 & val2.member;");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::amp, TT_BinaryOperator);
+
+  Tokens = annotate("val1 & val2.*member;");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::amp, TT_BinaryOperator);
+
+  Tokens = annotate("val1.*member & val2;");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::amp, TT_BinaryOperator);
+
+  Tokens = annotate("val1 & val2->*member;");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::amp, TT_BinaryOperator);
+
+  Tokens = annotate("val1->member & val2;");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::amp, TT_BinaryOperator);
+
+  Tokens = annotate("val1 & val2 & val3;");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::amp, TT_BinaryOperator);
+  EXPECT_TOKEN(Tokens[3], tok::amp, TT_BinaryOperator);
+
+  Tokens = annotate("val1 & val2 // comment\n"
+" & val3;");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::amp, TT_BinaryOperator);
+  EXPECT_TOKEN(Tokens[4], tok::amp, TT_BinaryOperator);
+
+  Tokens =
+  annotate("val1 & val2.member & val3.member() & val4 & val5->member;");
+  ASSERT_EQ(Tokens.size(), 19u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::amp, TT_BinaryOperator);
+  EXPECT_TOKEN(Tokens[5], tok::amp, TT_BinaryOperator);
+  EXPECT_TOKEN(Tokens[11], tok::amp, TT_BinaryOperator);
+  EXPECT_TOKEN(Tokens[13], tok::amp, TT_BinaryOperator);
+
+  Tokens = annotate("class c {\n"
+"  void func(type &a) { a & member; }\n"
+"  anotherType &member;\n"
+"}");
+  ASSERT_EQ(Tokens.size(), 22u) << Tokens;
+  EXPECT_TOKEN(Tokens[7], tok::amp, TT_PointerOrReference);
+  EXPECT_TOKEN(Tokens[12], tok::amp, TT_BinaryOperator);
+  EXPECT_TOKEN(Tokens[17], tok::amp, TT_PointerOrReference);
+
+  Tokens = annotate("struct S {\n"
+"  auto Mem = C & D;\n"
+"}");
+  ASSERT_EQ(Tokens.size(), 12u) << Tokens;
+  EXPECT_TOKEN(Tokens[7], tok::amp, TT_BinaryOperator);
 }
 
 TEST_F(TokenAnnotatorTest, UnderstandsUsesOfPlusAndMinus) {
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -11267,6 +11267,13 @@
 
   verifyFormat("int operator()(T (&&)[N]) { return 1; }");
   verifyFormat("int operator()(T (&)[N]) { return 0; }");
+
+  verifyFormat("val1 & val2;");
+  verifyFormat("val1 & val2 & val3;");
+  verifyFormat("class c {\n"
+   "  void func(type &a) { a & member; }\n"
+   "  anotherType &member;\n"
+   "}");
 }
 
 TEST_F(FormatTest, UnderstandsAttributes) {
Index: clang/lib/Format/TokenAnnotator.h
===
--- clang/lib/Format/TokenAnnotator.h
+++ clang/lib/Format/TokenAnnotator.h
@@ -34,6 +34,15 @@
   LT_CommentAbovePPDirective,
 };
 
+enum ScopeType {
+  // Contained in class declaration/definition.
+  ST_Class,
+  // Contained within function definition.
+  ST_Function,
+  // Contained within other scope block (loop, if/else, etc).
+  ST_Other,
+};
+
 class AnnotatedLine {
 public:
   AnnotatedLine(const UnwrappedLine &Line)
@@ -178,7 +187,7 @@
   // FIXME: Can/should this be done in the UnwrappedLineParser?
   void setCommentLineLevels(SmallVectorImpl &Lines) const;
 
-  void annotate(AnnotatedLine &Line) const;
+  void annotate(AnnotatedLine &Line);
   void calculateFormattingInformation(AnnotatedLine &Line) const;
 
 private:
@@ -

[clang] 3d25896 - [AST] Use std::clamp (NFC)

[Kazu Hirata via cfe-commits]

Author: Kazu Hirata
Date: 2023-01-29T09:43:36-08:00
New Revision: 3d25896bd97cc0f86d5861ec123a6b9556727b6c

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

LOG: [AST] Use std::clamp (NFC)

Added: 


Modified: 
clang/lib/AST/ASTContext.cpp

Removed: 




diff  --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 9ff1216548c19..aff3dec118dc7 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -2393,10 +2393,8 @@ TypeInfo ASTContext::getTypeInfoImpl(const Type *T) 
const {
   }
   case Type::BitInt: {
 const auto *EIT = cast(T);
-Align =
-std::min(static_cast(std::max(
- getCharWidth(), llvm::PowerOf2Ceil(EIT->getNumBits(,
- Target->getLongLongAlign());
+Align = std::clamp(llvm::PowerOf2Ceil(EIT->getNumBits()),
+ getCharWidth(), Target->getLongLongAlign());
 Width = llvm::alignTo(EIT->getNumBits(), Align);
 break;
   }



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

[PATCH] D142704: [C++20][Modules] Handle template declarations in header units.

[Iain Sandoe via Phabricator via cfe-commits]

iains added a comment.

AFAICT the failing test is unrelated to this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142704

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

[PATCH] D141144: [clang-tidy][doc] Improve clang-tidy documentation

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 493110.
carlosgalvezp added a comment.

Wrap trimming functionality in desc function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141144

Files:
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  clang-tools-extra/docs/clang-tidy/index.rst

Index: clang-tools-extra/docs/clang-tidy/index.rst
===
--- clang-tools-extra/docs/clang-tidy/index.rst
+++ clang-tools-extra/docs/clang-tidy/index.rst
@@ -122,8 +122,7 @@
 
   clang-tidy options:
 
---checks=  -
- Comma-separated list of globs with optional '-'
+--checks=  - Comma-separated list of globs with optional '-'
  prefix. Globs are processed in order of
  appearance in the list. Globs without '-'
  prefix add checks with matching names to the
@@ -132,21 +131,18 @@
  checks. This option's value is appended to the
  value of the 'Checks' option in .clang-tidy
  file, if any.
---config=  -
- Specifies a configuration in YAML/JSON format:
+--config=  - Specifies a configuration in YAML/JSON format:
-config="{Checks: '*',
- CheckOptions: {x, y}}"
+ CheckOptions: {x: y}}"
  When the value is empty, clang-tidy will
  attempt to find a file named .clang-tidy for
  each source file in its parent directories.
---config-file= - 
-Specify the path of .clang-tidy or custom config file:
-  e.g. --config-file=/some/path/myTidyConfigFile
-This option internally works exactly the same way as
+--config-file= - Specify the path of .clang-tidy or custom config file:
+ e.g. --config-file=/some/path/myTidyConfigFile
+ This option internally works exactly the same way as
   --config option after reading specified config file.
-Use either --config-file or --config, not both.
---dump-config  -
- Dumps configuration in the YAML format to
+ Use either --config-file or --config, not both.
+--dump-config  - Dumps configuration in the YAML format to
  stdout. This option can be used along with a
  file name (and '--' if the file is outside of a
  project with configured compilation database).
@@ -154,38 +150,29 @@
  printed.
  Use along with -checks=* to include
  configuration of all checks.
---enable-check-profile -
- Enable per-check timing profiles, and print a
+--enable-check-profile - Enable per-check timing profiles, and print a
  report to stderr.
---explain-config   -
- For each enabled check explains, where it is
+--explain-config   - For each enabled check explains, where it is
  enabled, i.e. in clang-tidy binary, command
  line or a specific configuration file.
---export-fixes=  -
- YAML file to store suggested fixes in. The
+--export-fixes=  - YAML file to store suggested fixes in. The
  stored fixes can be applied to the input source
  code with clang-apply-replacements.
---extra-arg=   - Additional argument to append to the compiler command line.
- Can be used several times.
---extra-arg-before=- Additional argument to prepend to the compiler command line.
- Can be used several times.
---fix  -
- Apply suggested fixes. Without -fix-errors
+--extra-arg=   - Additional argument to append to the compiler command line
+--extra-arg-before=- Additional argument to prepend to the

[PATCH] D141144: [clang-tidy][doc] Improve clang-tidy documentation

[Carlos Galvez via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:34
 
+static StringRef TrimFirstChar(StringRef x) { return x.substr(1); }
+

ClockMan wrote:
> Looks like usecase of this is more like rtrim, then probably rtrim should be 
> used instead of substr.
> And probably better would be to replace entire cl::desc. instead of adding 
> new additional wrap.
> 
> more like: 
> static auto desc(llvm::StringRef description) { return 
> cl::desc(description.rtrim()); }
Really good idea, much better! Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141144

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

[PATCH] D142804: [clang-format] Support clang-format on/off line comments as prefixes

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.



Comment at: clang/lib/Format/Format.cpp:3901
+  return Comment.startswith(Prefix) &&
+ (Comment.size() == Size || isblank(Comment[Size]));
+}

owenpan wrote:
> HazardyKnusperkeks wrote:
> > rymiel wrote:
> > > Should the space be required? What about `// clang-format off: reasoning` 
> > > or similar?
> > > 
> > > Also, should this be documented?
> > > Should the space be required? What about `// clang-format off: reasoning` 
> > > or similar?
> > > 
> > > Also, should this be documented?
> > 
> > +1
> > Should the space be required? What about `// clang-format off: reasoning` 
> > or similar?
> 
> On second thought, we should make it more restrictive to avoid regressions. 
> How about //requiring// a colon, i.e. `// clang-format off:` (but not `// 
> clang-format off :`)?
>  
> > Also, should this be documented?
> 
> Yep.
> 
> > Should the space be required? What about `// clang-format off: reasoning` 
> > or similar?
> 
> On second thought, we should make it more restrictive to avoid regressions. 
> How about //requiring// a colon, i.e. `// clang-format off:` (but not `// 
> clang-format off :`)?
>  

That's fine by me. But why not also `/**/`?


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

https://reviews.llvm.org/D142804

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

[PATCH] D142733: Add _Optional as fast qualifier

[Christopher Bazley via Phabricator via cfe-commits]

chrisbazley added a comment.

Hi, thanks very much for looking at my patch. I added the link that you 
proposed to all of the patches in the stack.

> Assuming that we want to go in this direction, it seems quite expensive to 
> model this as a fast qualifier rather than an extended qualifier.

True, and I'm not at all wedded to the idea of _Optional being a fast 
qualifier. I added it in the simplest way I knew how, having no prior 
experience of the codebase. What would be really useful would be if you could 
show/explain how to add it in a less risky way. If not, I can look into it 
myself.

> Are these annotations expected to be so common that it's better to increase 
> the alignment of all types than perform extra allocations and indirections 
> for _Optional qualifiers?

Given time, I hope so, but realistically not in the near future.

> Have you measured the memory impact of increasing the alignment of Type?

No, because I didn't think it necessary for the purpose of prototyping. If 
there's any prospect of getting my patches merged then I'd be delighted to 
invest the time... but only if increasing the alignment is a necessary thing to 
do.

> I think that should be a prerequisite to adding any new kind of fast 
> qualifier, and if we do add such a qualifier, we should select carefully 
> which qualifier gets this precious bit in QualType.

I believe that over time, _Optional would be a much more appropriate use of the 
precious bit currently occupied by volatile because I expect it to be used more 
heavily, but I can't make that argument yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142733

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

[PATCH] D141705: [HLSL] [Dirver] add dxv as a Driver Action Job

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:687
+def warn_drv_dxc_missing_dxv : Warning<"dxv not found."
+" Resulting DXIL will not be signed for use in release environments.">;
 

python3kgae wrote:
> beanz wrote:
> > Not all `dxv` binaries can sign… some just validate. Only the “official” 
> > releases support signing.
> Fixed.
Make sure there is a space after `.`. The diagnostics convention does not 
include the trailing period.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141705

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

[PATCH] D142595: [Driver][AVR] Don't emit default '-Tdata' when a linker script is specified

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

OK. If this doesn't add `-Tdata=` driver options, I'm fine with it.
But why is default `-Tdata` added in the first place?

Most linker scripts are added as `-Wl,-T,a.lds` (`-Wl,` values are opaque to 
the driver), so the driver cannot really know whether a linker script is used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142595

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

[PATCH] D141705: [HLSL] [Dirver] add dxv as a Driver Action Job

[Xiang Li via Phabricator via cfe-commits]

python3kgae updated this revision to Diff 493149.
python3kgae marked 2 inline comments as done.
python3kgae added a comment.

add space for warning message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141705

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Driver/Action.h
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/Types.def
  clang/lib/Driver/Action.cpp
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/HLSL.cpp
  clang/lib/Driver/ToolChains/HLSL.h
  clang/test/Driver/dxc_D.hlsl
  clang/test/Driver/dxc_I.hlsl
  clang/test/Driver/dxc_dxv_path.hlsl
  clang/unittests/Driver/DXCModeTest.cpp

Index: clang/unittests/Driver/DXCModeTest.cpp
===
--- clang/unittests/Driver/DXCModeTest.cpp
+++ clang/unittests/Driver/DXCModeTest.cpp
@@ -34,7 +34,7 @@
 DiagnosticsEngine &Diags) {
   Driver TheDriver("/bin/clang", "", Diags, "", InMemoryFileSystem);
   std::unique_ptr C{TheDriver.BuildCompilation(
-  {"clang", "--driver-mode=dxc", TargetProfile.data(), "foo.hlsl"})};
+  {"clang", "--driver-mode=dxc", TargetProfile.data(), "foo.hlsl", "-Vd"})};
   EXPECT_TRUE(C);
   EXPECT_STREQ(TheDriver.getTargetTriple().c_str(), ExpectTriple.data());
   EXPECT_EQ(Diags.getNumErrors(), 0u);
@@ -47,7 +47,7 @@
 unsigned NumOfErrors) {
   Driver TheDriver("/bin/clang", "", Diags, "", InMemoryFileSystem);
   std::unique_ptr C{TheDriver.BuildCompilation(
-  {"clang", "--driver-mode=dxc", TargetProfile.data(), "foo.hlsl"})};
+  {"clang", "--driver-mode=dxc", TargetProfile.data(), "foo.hlsl", "-Vd"})};
   EXPECT_TRUE(C);
   EXPECT_EQ(Diags.getNumErrors(), NumOfErrors);
   EXPECT_STREQ(DiagConsumer->Errors.back().c_str(), ExpectError.data());
Index: clang/test/Driver/dxc_dxv_path.hlsl
===
--- /dev/null
+++ clang/test/Driver/dxc_dxv_path.hlsl
@@ -0,0 +1,23 @@
+// RUN: %clang_dxc -I test -Tlib_6_3  -### %s 2>&1 | FileCheck %s
+
+// Make sure report warning.
+// CHECK:dxv not found.
+
+// RUN: echo "dxv" > %T/dxv && chmod 754 %T/dxv && %clang_dxc --dxv-path=%T %s -Tlib_6_3 -### 2>&1 | FileCheck %s --check-prefix=DXV_PATH
+// DXV_PATH:dxv{{(.exe)?}}" "-" "-o" "-"
+
+// RUN: %clang_dxc -I test -Vd -Tlib_6_3  -### %s 2>&1 | FileCheck %s --check-prefix=VD
+// VD:"-cc1"{{.*}}"-triple" "dxil-unknown-shadermodel6.3-library"
+// VD-NOT:dxv not found
+
+// RUN: %clang_dxc -Tlib_6_3 -ccc-print-bindings --dxv-path=%T -Fo %t.dxo  %s 2>&1 | FileCheck %s --check-prefix=BINDINGS
+// BINDINGS: "dxil-unknown-shadermodel6.3-library" - "clang", inputs: ["[[INPUT:.+]]"], output: "[[DXC:.+]].dxo"
+// BINDINGS-NEXT: "dxil-unknown-shadermodel6.3-library" - "hlsl::Validator", inputs: ["[[DXC]].dxo"], output: "[[DXC]].dxo"
+
+// RUN: %clang_dxc -Tlib_6_3 -ccc-print-phases --dxv-path=%T -Fo %t.dxc  %s 2>&1 | FileCheck %s --check-prefix=PHASES
+
+// PHASES: 0: input, "[[INPUT:.+]]", hlsl
+// PHASES-NEXT: 1: preprocessor, {0}, c++-cpp-output
+// PHASES-NEXT: 2: compiler, {1}, ir
+// PHASES-NEXT: 3: backend, {2}, assembler
+// PHASES-NEXT: 4: binary-analyzer, {3}, dx-container
Index: clang/test/Driver/dxc_I.hlsl
===
--- clang/test/Driver/dxc_I.hlsl
+++ clang/test/Driver/dxc_I.hlsl
@@ -1,4 +1,4 @@
-// RUN: %clang_dxc -I test  -### %s 2>&1 | FileCheck %s
+// RUN: %clang_dxc -I test -Tlib_6_3  -### %s 2>&1 | FileCheck %s
 
 // Make sure -I send to cc1.
 // CHECK:"-I" "test"
Index: clang/test/Driver/dxc_D.hlsl
===
--- clang/test/Driver/dxc_D.hlsl
+++ clang/test/Driver/dxc_D.hlsl
@@ -1,4 +1,4 @@
-// RUN: %clang_dxc -DTEST=2  -### %s 2>&1 | FileCheck %s
+// RUN: %clang_dxc -DTEST=2 -Tlib_6_7 -### %s 2>&1 | FileCheck %s
 // RUN: %clang_dxc -DTEST=2  -Tlib_6_7 %s -fcgl -Fo - | FileCheck %s --check-prefix=ERROR
 
 // Make sure -D send to cc1.
Index: clang/lib/Driver/ToolChains/HLSL.h
===
--- clang/lib/Driver/ToolChains/HLSL.h
+++ clang/lib/Driver/ToolChains/HLSL.h
@@ -9,17 +9,37 @@
 #ifndef LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_HLSL_H
 #define LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_HLSL_H
 
+#include "clang/Driver/Tool.h"
 #include "clang/Driver/ToolChain.h"
 
 namespace clang {
 namespace driver {
 
+namespace tools {
+
+namespace hlsl {
+class LLVM_LIBRARY_VISIBILITY Validator : public Tool {
+public:
+  Validator(const ToolChain &TC) : Tool("hlsl::Validator", "dxv", TC) {}
+
+  bool hasIntegratedCPP() const override { return false; }
+
+  void ConstructJob(Compilation &C, const JobAction &JA,
+const InputInfo &Output, const InputInfoList &Inputs,
+const llvm::opt::ArgList &TCArgs

[PATCH] D141705: [HLSL] [Dirver] add dxv as a Driver Action Job

[Xiang Li via Phabricator via cfe-commits]

python3kgae added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:687
+def warn_drv_dxc_missing_dxv : Warning<"dxv not found."
+" Resulting DXIL will not be signed for use in release environments.">;
 

MaskRay wrote:
> python3kgae wrote:
> > beanz wrote:
> > > Not all `dxv` binaries can sign… some just validate. Only the “official” 
> > > releases support signing.
> > Fixed.
> Make sure there is a space after `.`. The diagnostics convention does not 
> include the trailing period.
Good catch!
Fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141705

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

[clang] 7a063c1 - [AST] Use llvm::bit_ceil (NFC)

[Kazu Hirata via cfe-commits]

Author: Kazu Hirata
Date: 2023-01-29T15:39:46-08:00
New Revision: 7a063c1a13b209425ef1611a5f08e2c500ac7f30

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

LOG: [AST] Use llvm::bit_ceil (NFC)

For nonzero X that is not a power of 2, NextPowerOf2(X) is equivalent
to llvm::bit_ceil(X).

Added: 


Modified: 
clang/lib/AST/ASTContext.cpp

Removed: 




diff  --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index aff3dec118dc..5976ada29a86 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -2084,7 +2084,7 @@ TypeInfo ASTContext::getTypeInfoImpl(const Type *T) const 
{
 // If the alignment is not a power of 2, round up to the next power of 2.
 // This happens for non-power-of-2 length vectors.
 if (Align & (Align-1)) {
-  Align = llvm::NextPowerOf2(Align);
+  Align = llvm::bit_ceil(Align);
   Width = llvm::alignTo(Width, Align);
 }
 // Adjust the alignment based on the target max.



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

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

@daiyousei-qz what is the current status of this patch? Is it ready to be 
merged again? (If so, I can do that for you.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127082

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

[PATCH] D142857: [clang] Remove clang::Optional

[Kazu Hirata via Phabricator via cfe-commits]

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

This is part of an effort to migrate from llvm::Optional to
std::optional:

https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142857

Files:
  clang/include/clang/Basic/LLVM.h


Index: clang/include/clang/Basic/LLVM.h
===
--- clang/include/clang/Basic/LLVM.h
+++ clang/include/clang/Basic/LLVM.h
@@ -19,9 +19,6 @@
 // dependencies.
 // Casting.h has complex templates that cannot be easily forward declared.
 #include "llvm/Support/Casting.h"
-// None.h includes an enumerator that is desired & cannot be forward declared
-// without a definition of NoneType.
-#include "llvm/ADT/None.h"
 // Add this header as a workaround to prevent `too few template arguments for
 // class template 'SmallVector'` building error with build compilers like XL.
 #include "llvm/ADT/SmallVector.h"
@@ -37,7 +34,6 @@
   template class SmallString;
   template class SmallVector;
   template class SmallVectorImpl;
-  template  using Optional = std::optional;
   template  class Expected;
 
   template
@@ -69,7 +65,6 @@
   // ADT's.
   using llvm::ArrayRef;
   using llvm::MutableArrayRef;
-  using llvm::Optional;
   using llvm::OwningArrayRef;
   using llvm::SaveAndRestore;
   using llvm::SmallString;


Index: clang/include/clang/Basic/LLVM.h
===
--- clang/include/clang/Basic/LLVM.h
+++ clang/include/clang/Basic/LLVM.h
@@ -19,9 +19,6 @@
 // dependencies.
 // Casting.h has complex templates that cannot be easily forward declared.
 #include "llvm/Support/Casting.h"
-// None.h includes an enumerator that is desired & cannot be forward declared
-// without a definition of NoneType.
-#include "llvm/ADT/None.h"
 // Add this header as a workaround to prevent `too few template arguments for
 // class template 'SmallVector'` building error with build compilers like XL.
 #include "llvm/ADT/SmallVector.h"
@@ -37,7 +34,6 @@
   template class SmallString;
   template class SmallVector;
   template class SmallVectorImpl;
-  template  using Optional = std::optional;
   template  class Expected;
 
   template
@@ -69,7 +65,6 @@
   // ADT's.
   using llvm::ArrayRef;
   using llvm::MutableArrayRef;
-  using llvm::Optional;
   using llvm::OwningArrayRef;
   using llvm::SaveAndRestore;
   using llvm::SmallString;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142857: [clang] Remove clang::Optional

[Fangrui Song via Phabricator via cfe-commits]

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

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142857

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

[PATCH] D142595: [Driver][AVR] Don't emit default '-Tdata' when a linker script is specified

[Ben Shi via Phabricator via cfe-commits]

benshi001 added a comment.

In D142595#4089124 , @MaskRay wrote:

> OK. If this doesn't add `-Tdata=` driver options, I'm fine with it.
> But why is default `-Tdata` added in the first place?
>
> Most linker scripts are added as `-Wl,-T,a.lds` (`-Wl,` values are opaque to 
> the driver), so the driver cannot really know whether a linker script is used.

So how about change to

1. move the default `-Tdata` to a later position.
2. clang driver just checks `-T` but omits `-Wl`, this can not 100% fix the 
issue, but at least improve it. Current most users are from avr-gcc, who have 
get used to `-T`.

A good solution is using default linker script provided by the avr-libc, in 
which all sections start addresses are undefined symbols, which need users 
define them via command line options. -- I will do that solution in a different 
patch. Currently we just guarantee avr-gcc users's `-T` option will not be 
broken.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142595

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

[clang] 125f445 - [clang] Remove clang::Optional

[Kazu Hirata via cfe-commits]

Author: Kazu Hirata
Date: 2023-01-29T18:08:00-08:00
New Revision: 125f4457a54a550846732763ee36b1447ec8d66e

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

LOG: [clang] Remove clang::Optional

This is part of an effort to migrate from llvm::Optional to
std::optional:

https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716

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

Added: 


Modified: 
clang/include/clang/Basic/LLVM.h

Removed: 




diff  --git a/clang/include/clang/Basic/LLVM.h 
b/clang/include/clang/Basic/LLVM.h
index 7ffc4c403473b..f4956cd16cbcf 100644
--- a/clang/include/clang/Basic/LLVM.h
+++ b/clang/include/clang/Basic/LLVM.h
@@ -19,9 +19,6 @@
 // dependencies.
 // Casting.h has complex templates that cannot be easily forward declared.
 #include "llvm/Support/Casting.h"
-// None.h includes an enumerator that is desired & cannot be forward declared
-// without a definition of NoneType.
-#include "llvm/ADT/None.h"
 // Add this header as a workaround to prevent `too few template arguments for
 // class template 'SmallVector'` building error with build compilers like XL.
 #include "llvm/ADT/SmallVector.h"
@@ -37,7 +34,6 @@ namespace llvm {
   template class SmallString;
   template class SmallVector;
   template class SmallVectorImpl;
-  template  using Optional = std::optional;
   template  class Expected;
 
   template
@@ -69,7 +65,6 @@ namespace clang {
   // ADT's.
   using llvm::ArrayRef;
   using llvm::MutableArrayRef;
-  using llvm::Optional;
   using llvm::OwningArrayRef;
   using llvm::SaveAndRestore;
   using llvm::SmallString;



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

[PATCH] D142857: [clang] Remove clang::Optional

[Kazu Hirata 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 rG125f4457a54a: [clang] Remove clang::Optional (authored by 
kazu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142857

Files:
  clang/include/clang/Basic/LLVM.h


Index: clang/include/clang/Basic/LLVM.h
===
--- clang/include/clang/Basic/LLVM.h
+++ clang/include/clang/Basic/LLVM.h
@@ -19,9 +19,6 @@
 // dependencies.
 // Casting.h has complex templates that cannot be easily forward declared.
 #include "llvm/Support/Casting.h"
-// None.h includes an enumerator that is desired & cannot be forward declared
-// without a definition of NoneType.
-#include "llvm/ADT/None.h"
 // Add this header as a workaround to prevent `too few template arguments for
 // class template 'SmallVector'` building error with build compilers like XL.
 #include "llvm/ADT/SmallVector.h"
@@ -37,7 +34,6 @@
   template class SmallString;
   template class SmallVector;
   template class SmallVectorImpl;
-  template  using Optional = std::optional;
   template  class Expected;
 
   template
@@ -69,7 +65,6 @@
   // ADT's.
   using llvm::ArrayRef;
   using llvm::MutableArrayRef;
-  using llvm::Optional;
   using llvm::OwningArrayRef;
   using llvm::SaveAndRestore;
   using llvm::SmallString;


Index: clang/include/clang/Basic/LLVM.h
===
--- clang/include/clang/Basic/LLVM.h
+++ clang/include/clang/Basic/LLVM.h
@@ -19,9 +19,6 @@
 // dependencies.
 // Casting.h has complex templates that cannot be easily forward declared.
 #include "llvm/Support/Casting.h"
-// None.h includes an enumerator that is desired & cannot be forward declared
-// without a definition of NoneType.
-#include "llvm/ADT/None.h"
 // Add this header as a workaround to prevent `too few template arguments for
 // class template 'SmallVector'` building error with build compilers like XL.
 #include "llvm/ADT/SmallVector.h"
@@ -37,7 +34,6 @@
   template class SmallString;
   template class SmallVector;
   template class SmallVectorImpl;
-  template  using Optional = std::optional;
   template  class Expected;
 
   template
@@ -69,7 +65,6 @@
   // ADT's.
   using llvm::ArrayRef;
   using llvm::MutableArrayRef;
-  using llvm::Optional;
   using llvm::OwningArrayRef;
   using llvm::SaveAndRestore;
   using llvm::SmallString;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141910: [OpenMP][OMPIRBuilder]Move SIMD alignment calculation to LLVM Frontend

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

Hi @domada, these changes break compilation of clang, with such build error:

  FAILED: tools/clang/lib/AST/CMakeFiles/obj.clangAST.dir/ASTContext.cpp.o 
  
  In file included from /llvm-project/clang/lib/AST/ASTContext.cpp:81:
  In file included from 
/llvm-project/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:17:
  In file included from 
/llvm-project/llvm/include/llvm/Analysis/MemorySSAUpdater.h:37:
  In file included from /llvm-project/llvm/include/llvm/Analysis/MemorySSA.h:93:
  In file included from 
/llvm-project/llvm/include/llvm/Analysis/AliasAnalysis.h:44:
  In file included from /llvm-project/llvm/include/llvm/IR/PassManager.h:45:
  In file included from /llvm-project/llvm/include/llvm/IR/Function.h:25:
  In file included from /llvm-project/llvm/include/llvm/IR/Argument.h:17:
  /llvm-project/llvm/include/llvm/IR/Attributes.h:90:14: fatal error: 
'llvm/IR/Attributes.inc' file not found

Reproduction steps:

1. Configure to build clang using `ninja`:

  cmake -G Ninja /path/to/llvm-project/llvm \
-DLLVM_TARGETS_TO_BUILD="X86;ARM;AArch64" \
-DCMAKE_BUILD_TYPE:STRING=Release \
-DLLVM_ENABLE_PROJECTS="clang"

2. Build `ASTContext.cpp.o` with a clean build directory.

  ninja clean && ninja 
tools/clang/lib/AST/CMakeFiles/obj.clangAST.dir/ASTContext.cpp.o

Are you able to take a look? 🙏


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141910

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

[PATCH] D142595: [Driver][AVR] Don't emit default '-Tdata' when '-T' is specified

[Ben Shi via Phabricator via cfe-commits]

benshi001 updated this revision to Diff 493179.
benshi001 retitled this revision from "[Driver][AVR] Don't emit default 
'-Tdata' when a linker script is specified" to "[Driver][AVR] Don't emit 
default '-Tdata' when '-T' is specified".
benshi001 edited the summary of this revision.
benshi001 removed a reviewer: Miss_Grape.

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

https://reviews.llvm.org/D142595

Files:
  clang/lib/Driver/ToolChains/AVR.cpp
  clang/test/Driver/Inputs/basic_avr_tree/usr/lib/avr/lib/avr35.lds
  clang/test/Driver/Inputs/basic_avr_tree/usr/lib/avr/lib/avr51.lds
  clang/test/Driver/avr-ld.c
  clang/test/Driver/avr-toolchain.c

Index: clang/test/Driver/avr-toolchain.c
===
--- clang/test/Driver/avr-toolchain.c
+++ clang/test/Driver/avr-toolchain.c
@@ -7,8 +7,8 @@
 // CHECK1-SAME: "-isysroot" "[[SYSROOT:[^"]+/basic_avr_tree]]"
 // CHECK1-SAME: "-internal-isystem"
 // CHECK1-SAME: {{^}} "[[SYSROOT]]/usr/lib/gcc/avr/5.4.0/../../../avr/include"
-// CHECK1-NOT:  "-L
-// CHECK1:  avr-ld"
+// CHECK1-NOT:  "-L"
+// CHECK1:  {{".*avr-ld"}}
 // CHECK1-SAME: "-o" "a.out"
 // CHECK1-SAME: {{^}} "--gc-sections"
 
@@ -46,7 +46,6 @@
 // RUN: %clang -### --target=avr --sysroot=%S/Inputs/basic_avr_tree %s 2>&1 | FileCheck --check-prefixes=NOMCU,LINKB %s
 // NOMCU: warning: no target microcontroller specified on command line, cannot link standard libraries, please pass -mmcu=
 // LINKB: warning: standard library not linked and so no interrupt vector table or compiler runtime routines will be linked
-// LINKB: warning: support for passing the data section address to the linker for microcontroller '' is not implemented
 // NOMCU-NOT: warning: {{.*}} avr-gcc
 // NOMCU-NOT: warning: {{.*}} avr-libc
 // LINKA-NOT: warning: {{.*}} interrupt vector
Index: clang/test/Driver/avr-ld.c
===
--- clang/test/Driver/avr-ld.c
+++ clang/test/Driver/avr-ld.c
@@ -1,44 +1,52 @@
 // RUN: %clang -### --target=avr -mmcu=at90s2313 --rtlib=libgcc --sysroot %S/Inputs/basic_avr_tree %s 2>&1 | FileCheck -check-prefix LINKA %s
-// LINKA: {{".*ld.*"}} {{.*}} {{"-L.*tiny-stack"}} {{.*}} "-Tdata=0x800060" "--start-group" {{.*}} "-lat90s2313" {{.*}} "--end-group" "-mavr2"
+// LINKA: {{".*ld.*"}} {{.*}} {{"-L.*tiny-stack"}} {{.*}} "--start-group" {{.*}} "-lat90s2313" {{.*}} "--end-group" "-Tdata=0x800060" "-mavr2"
 
 // RUN: %clang -### --target=avr -mmcu=at90s8515 --rtlib=libgcc --sysroot %S/Inputs/basic_avr_tree %s 2>&1 | FileCheck -check-prefix LINKB %s
-// LINKB: {{".*ld.*"}} {{.*}} "-Tdata=0x800060" "--start-group" {{.*}} "-lat90s8515" {{.*}} "--end-group" "-mavr2"
+// LINKB: {{".*ld.*"}} {{.*}} "--start-group" {{.*}} "-lat90s8515" {{.*}} "--end-group" "-Tdata=0x800060" "-mavr2"
 
 // RUN: %clang -### --target=avr -mmcu=attiny13 --rtlib=libgcc --sysroot %S/Inputs/basic_avr_tree %s 2>&1 | FileCheck -check-prefix LINKC %s
-// LINKC: {{".*ld.*"}} {{.*}} {{"-L.*avr25/tiny-stack"}} {{.*}} "-Tdata=0x800060" "--start-group" {{.*}} "-lattiny13" {{.*}} "--end-group" "-mavr25"
+// LINKC: {{".*ld.*"}} {{.*}} {{"-L.*avr25/tiny-stack"}} {{.*}} "--start-group" {{.*}} "-lattiny13" {{.*}} "--end-group" "-Tdata=0x800060" "-mavr25"
 
 // RUN: %clang -### --target=avr -mmcu=attiny44 --rtlib=libgcc --sysroot %S/Inputs/basic_avr_tree %s 2>&1 | FileCheck -check-prefix LINKD %s
-// LINKD: {{".*ld.*"}} {{.*}} {{"-L.*avr25"}} {{.*}} "-Tdata=0x800060" "--start-group" {{.*}} "-lattiny44" {{.*}} "--end-group" "-mavr25"
+// LINKD: {{".*ld.*"}} {{.*}} {{"-L.*avr25"}} {{.*}} "--start-group" {{.*}} "-lattiny44" {{.*}} "--end-group" "-Tdata=0x800060" "-mavr25"
 
 // RUN: %clang -### --target=avr -mmcu=atmega103 --rtlib=libgcc --sysroot %S/Inputs/basic_avr_tree %s 2>&1 | FileCheck -check-prefix LINKE %s
-// LINKE: {{".*ld.*"}} {{.*}} {{"-L.*avr31"}} {{.*}} "-Tdata=0x800060" "--start-group" {{.*}} "-latmega103" {{.*}} "--end-group" "-mavr31"
+// LINKE: {{".*ld.*"}} {{.*}} {{"-L.*avr31"}} {{.*}} "--start-group" {{.*}} "-latmega103" {{.*}} "--end-group" "-Tdata=0x800060" "-mavr31"
 
 // RUN: %clang -### --target=avr -mmcu=atmega8u2 --rtlib=libgcc --sysroot %S/Inputs/basic_avr_tree %s 2>&1 | FileCheck -check-prefix LINKF %s
-// LINKF: {{".*ld.*"}} {{.*}} {{"-L.*avr35"}} {{.*}} "-Tdata=0x800100" "--start-group" {{.*}} "-latmega8u2" {{.*}} "--end-group" "-mavr35"
+// LINKF: {{".*ld.*"}} {{.*}} {{"-L.*avr35"}} {{.*}} "--start-group" {{.*}} "-latmega8u2" {{.*}} "--end-group" "-Tdata=0x800100" "-mavr35"
 
 // RUN: %clang -### --target=avr -mmcu=atmega48pa --rtlib=libgcc --sysroot %S/Inputs/basic_avr_tree %s 2>&1 | FileCheck -check-prefix LINKG %s
-// LINKG: {{".*ld.*"}} {{.*}} {{"-L.*avr4"}} {{.*}} "-Tdata=0x800100" "--start-group" {{.*}} "-latmega48pa" {{.*}} "--end-group" "-mavr4"
+// LINKG: {{".*ld.*"}} {{.*}} {{"-L.*avr4"}} {{.*}} "--start-group" {{.*}} "-latmega48pa" {{.*}} "--end-group" "-Tdata=0x800100" "-mavr4"
 
 //

[PATCH] D142595: [Driver][AVR] Don't emit default '-Tdata' when '-T' is specified

[Ben Shi via Phabricator via cfe-commits]

benshi001 added inline comments.



Comment at: clang/lib/Driver/ToolChains/AVR.cpp:544
+  if (SectionAddressData) {
+CmdArgs.push_back(Args.MakeArgString(
+"-Tdata=0x" + Twine::utohexstr(*SectionAddressData)));

Just moving the default `-T` from previous position to here, without any change.


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

https://reviews.llvm.org/D142595

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

[PATCH] D142804: [clang-format] Support clang-format on/off line comments as prefixes

[Owen Pan via Phabricator via cfe-commits]

owenpan updated this revision to Diff 493180.
owenpan added a comment.

Improves `isClangFormatOnOff()` a little bit.


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

https://reviews.llvm.org/D142804

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/FormatTokenLexer.cpp
  clang/lib/Format/IntegerLiteralSeparatorFixer.cpp
  clang/lib/Format/SortJavaScriptImports.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -22619,6 +22619,28 @@
"/* clang-format off */\n"
"/* long long long long long long line */\n",
getLLVMStyleWithColumns(20)));
+
+  verifyFormat("int *i;\n"
+   "// clang-format off:\n"
+   "int* j;\n"
+   "// clang-format on: 1\n"
+   "int *k;",
+   "int* i;\n"
+   "// clang-format off:\n"
+   "int* j;\n"
+   "// clang-format on: 1\n"
+   "int* k;");
+
+  verifyFormat("int *i;\n"
+   "// clang-format off:0\n"
+   "int* j;\n"
+   "// clang-format only\n"
+   "int* k;",
+   "int* i;\n"
+   "// clang-format off:0\n"
+   "int* j;\n"
+   "// clang-format only\n"
+   "int* k;");
 }
 
 TEST_F(FormatTest, DoNotCrashOnInvalidInput) {
Index: clang/lib/Format/SortJavaScriptImports.cpp
===
--- clang/lib/Format/SortJavaScriptImports.cpp
+++ clang/lib/Format/SortJavaScriptImports.cpp
@@ -195,8 +195,7 @@
 // Separate references from the main code body of the file.
 if (FirstNonImportLine && FirstNonImportLine->First->NewlinesBefore < 2 &&
 !(FirstNonImportLine->First->is(tok::comment) &&
-  FirstNonImportLine->First->TokenText.trim() ==
-  "// clang-format on")) {
+  isClangFormatOn(FirstNonImportLine->First->TokenText.trim( {
   ReferencesText += "\n";
 }
 
@@ -376,9 +375,9 @@
   // This is tracked in FormattingOff here and on JsModuleReference.
   while (Current && Current->is(tok::comment)) {
 StringRef CommentText = Current->TokenText.trim();
-if (CommentText == "// clang-format off") {
+if (isClangFormatOff(CommentText)) {
   FormattingOff = true;
-} else if (CommentText == "// clang-format on") {
+} else if (isClangFormatOn(CommentText)) {
   FormattingOff = false;
   // Special case: consider a trailing "clang-format on" line to be part
   // of the module reference, so that it gets moved around together with
Index: clang/lib/Format/IntegerLiteralSeparatorFixer.cpp
===
--- clang/lib/Format/IntegerLiteralSeparatorFixer.cpp
+++ clang/lib/Format/IntegerLiteralSeparatorFixer.cpp
@@ -87,9 +87,9 @@
 auto Location = Tok.getLocation();
 auto Text = StringRef(SourceMgr.getCharacterData(Location), Length);
 if (Tok.is(tok::comment)) {
-  if (Text == "// clang-format off" || Text == "/* clang-format off */")
+  if (isClangFormatOff(Text))
 Skip = true;
-  else if (Text == "// clang-format on" || Text == "/* clang-format on */")
+  else if (isClangFormatOn(Text))
 Skip = false;
   continue;
 }
Index: clang/lib/Format/FormatTokenLexer.cpp
===
--- clang/lib/Format/FormatTokenLexer.cpp
+++ clang/lib/Format/FormatTokenLexer.cpp
@@ -1286,17 +1286,13 @@
 Tok.Tok.setKind(tok::string_literal);
   }
 
-  if (Tok.is(tok::comment) && (Tok.TokenText == "// clang-format on" ||
-   Tok.TokenText == "/* clang-format on */")) {
+  if (Tok.is(tok::comment) && isClangFormatOn(Tok.TokenText))
 FormattingDisabled = false;
-  }
 
   Tok.Finalized = FormattingDisabled;
 
-  if (Tok.is(tok::comment) && (Tok.TokenText == "// clang-format off" ||
-   Tok.TokenText == "/* clang-format off */")) {
+  if (Tok.is(tok::comment) && isClangFormatOff(Tok.TokenText))
 FormattingDisabled = true;
-  }
 }
 
 void FormatTokenLexer::resetLexer(unsigned Offset) {
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -3002,13 +3002,10 @@
 if (Trimmed.contains(RawStringTermination))
   FormattingOff = false;
 
-if (Trimmed == "// clang-format off" ||
-Trimmed == "/* clang-format off */") {
+if (isClangFormatOff(Trimmed))
   FormattingOff = true;
-} else if (Trimmed == "// clang-format on" ||
-   Tri

[PATCH] D142804: [clang-format] Support clang-format on/off line comments as prefixes

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.



Comment at: clang/lib/Format/Format.cpp:3901
+  return Comment.startswith(Prefix) &&
+ (Comment.size() == Size || isblank(Comment[Size]));
+}

HazardyKnusperkeks wrote:
> owenpan wrote:
> > HazardyKnusperkeks wrote:
> > > rymiel wrote:
> > > > Should the space be required? What about `// clang-format off: 
> > > > reasoning` or similar?
> > > > 
> > > > Also, should this be documented?
> > > > Should the space be required? What about `// clang-format off: 
> > > > reasoning` or similar?
> > > > 
> > > > Also, should this be documented?
> > > 
> > > +1
> > > Should the space be required? What about `// clang-format off: reasoning` 
> > > or similar?
> > 
> > On second thought, we should make it more restrictive to avoid regressions. 
> > How about //requiring// a colon, i.e. `// clang-format off:` (but not `// 
> > clang-format off :`)?
> >  
> > > Also, should this be documented?
> > 
> > Yep.
> > 
> > > Should the space be required? What about `// clang-format off: reasoning` 
> > > or similar?
> > 
> > On second thought, we should make it more restrictive to avoid regressions. 
> > How about //requiring// a colon, i.e. `// clang-format off:` (but not `// 
> > clang-format off :`)?
> >  
> 
> That's fine by me. But why not also `/**/`?
> But why not also `/**/`?

If it weren't for the fact that the code which checks for clang-format on/off 
exists in several places, I wouldn't want this feature added. IMO there's no 
need to allow `/* clang-format off: */` if we got `// clang-format:` and being 
more restrictive results in a lower risk of regression.


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

https://reviews.llvm.org/D142804

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

[PATCH] D142861: [Clang] avoid relying on StringMap iteration order when roundtripping -analyzer-config

[Erik Desjardins via Phabricator via cfe-commits]

erikdesjardins created this revision.
Herald added subscribers: manas, ASDenysPetrov, dkrupp, donat.nagy, Szelethus, 
a.sidorin, mgrang, baloghadamsoftware.
Herald added a project: All.
erikdesjardins requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

I am working on another patch that changes StringMap's hash function,
which changes the iteration order here, and breaks some tests,
specifically:

  clang/test/Analysis/NSString.m
  clang/test/Analysis/shallow-mode.m

with errors like:

  generated arguments do not match in round-trip
  generated arguments #1 in round-trip: <...> "-analyzer-config" "ipa=inlining" 
"-analyzer-config" "max-nodes=75000" <...>
  generated arguments #2 in round-trip: <...> "-analyzer-config" 
"max-nodes=75000" "-analyzer-config" "ipa=inlining" <...>

To avoid this, sort the options by key, instead of using the default map
iteration order.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142861

Files:
  clang/lib/Frontend/CompilerInvocation.cpp


Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -877,14 +877,25 @@
   AnalyzerOptions ConfigOpts;
   parseAnalyzerConfigs(ConfigOpts, nullptr);
 
+  // Sort options by key to avoid relying on StringMap iteration order.
+  SmallVector, 4> SortedConfigOpts;
   for (const auto &C : Opts.Config) {
+SortedConfigOpts.emplace_back(C.getKey(), C.getValue());
+  }
+  llvm::sort(SortedConfigOpts, [](const auto &A, const auto &B) {
+return std::get<0>(A) < std::get<0>(B);
+  });
+
+  for (const auto &C : SortedConfigOpts) {
+const llvm::StringRef &Key = std::get<0>(C);
+const std::string &Value = std::get<1>(C);
 // Don't generate anything that came from parseAnalyzerConfigs. It would be
 // redundant and may not be valid on the command line.
-auto Entry = ConfigOpts.Config.find(C.getKey());
-if (Entry != ConfigOpts.Config.end() && Entry->getValue() == C.getValue())
+auto Entry = ConfigOpts.Config.find(Key);
+if (Entry != ConfigOpts.Config.end() && Entry->getValue() == Value)
   continue;
 
-GenerateArg(Args, OPT_analyzer_config, C.getKey() + "=" + C.getValue(), 
SA);
+GenerateArg(Args, OPT_analyzer_config, Key + "=" + Value, SA);
   }
 
   // Nothing to generate for FullCompilerInvocation.


Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -877,14 +877,25 @@
   AnalyzerOptions ConfigOpts;
   parseAnalyzerConfigs(ConfigOpts, nullptr);
 
+  // Sort options by key to avoid relying on StringMap iteration order.
+  SmallVector, 4> SortedConfigOpts;
   for (const auto &C : Opts.Config) {
+SortedConfigOpts.emplace_back(C.getKey(), C.getValue());
+  }
+  llvm::sort(SortedConfigOpts, [](const auto &A, const auto &B) {
+return std::get<0>(A) < std::get<0>(B);
+  });
+
+  for (const auto &C : SortedConfigOpts) {
+const llvm::StringRef &Key = std::get<0>(C);
+const std::string &Value = std::get<1>(C);
 // Don't generate anything that came from parseAnalyzerConfigs. It would be
 // redundant and may not be valid on the command line.
-auto Entry = ConfigOpts.Config.find(C.getKey());
-if (Entry != ConfigOpts.Config.end() && Entry->getValue() == C.getValue())
+auto Entry = ConfigOpts.Config.find(Key);
+if (Entry != ConfigOpts.Config.end() && Entry->getValue() == Value)
   continue;
 
-GenerateArg(Args, OPT_analyzer_config, C.getKey() + "=" + C.getValue(), SA);
+GenerateArg(Args, OPT_analyzer_config, Key + "=" + Value, SA);
   }
 
   // Nothing to generate for FullCompilerInvocation.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142862: [Support] change StringMap hash function from djbHash to xxHash

[Erik Desjardins via Phabricator via cfe-commits]

erikdesjardins created this revision.
Herald added subscribers: Moerafaat, zero9178, Enna1, bzcheeseman, sdasgup3, 
wenzhicui, wrengr, cota, teijeong, rdzhabarov, tatianashp, wenlei, msifontes, 
jurahul, Kayjukh, grosul1, Joonsoo, liufengdb, aartbik, mgester, arpith-jacob, 
antiagainst, shauheen, rriddle, mehdi_amini, thopre, kadircet, arphaman, 
JDevlieghere, hiraditya.
Herald added a reviewer: JDevlieghere.
Herald added a project: All.
erikdesjardins requested review of this revision.
Herald added subscribers: cfe-commits, llvm-commits, lldb-commits, Sanitizers, 
stephenneuendorffer, nicolasvasilache.
Herald added projects: clang, Sanitizers, LLDB, MLIR, LLVM, clang-tools-extra.

Depends on https://reviews.llvm.org/D142861.

Alternative to https://reviews.llvm.org/D137601.

xxHash is much faster than djbHash. This makes a simple Rust test case with a 
large constant string 10% faster to compile.

Previous attempts at changing this hash function (e.g. 
https://reviews.llvm.org/D97396) had to be reverted due to breaking tests that 
depended on iteration order.
No additional tests fail with this patch compared to `main` when running 
`check-all` with `-DLLVM_ENABLE_PROJECTS="all"` (on a Linux host), so I hope I 
found everything that needs to be changed.

For posterity, the tests that fail on main are:

  ompd-test :: api_tests/test_ompd_finalize.c
  ompd-test :: api_tests/test_ompd_get_curr_parallel_handle.c

Overall:

  Skipped  : 43
  Unsupported  :   2862
  Passed   : 100507
  Expectedly Failed:296
  Failed   :  2


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142862

Files:
  clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
  clang-tools-extra/test/modularize/ProblemsDisplayLists.modularize
  clang-tools-extra/test/modularize/ProblemsInconsistent.modularize
  clang/unittests/Basic/SarifTest.cpp
  compiler-rt/test/profile/Linux/instrprof-show-debug-info-correlation.c
  lldb/test/API/functionalities/gdb_remote_client/TestGDBRemoteClient.py
  llvm/lib/Support/StringMap.cpp
  llvm/test/DebugInfo/Generic/accel-table-hash-collisions.ll
  llvm/test/DebugInfo/Generic/debug-names-hash-collisions.ll
  llvm/test/DebugInfo/X86/debug-pubtables-dwarf64.ll
  llvm/test/DebugInfo/X86/gnu-public-names-gmlt.ll
  llvm/test/DebugInfo/X86/gnu-public-names.ll
  llvm/test/ObjectYAML/Offload/binary.yaml
  llvm/test/ObjectYAML/Offload/multiple_members.yaml
  llvm/test/tools/dsymutil/ARM/extern-alias.test
  llvm/test/tools/llvm-profdata/suppl-instr-with-sample.test
  mlir/test/Transforms/print-op-graph.mlir
  mlir/test/mlir-lsp-server/completion.test

Index: mlir/test/mlir-lsp-server/completion.test
===
--- mlir/test/mlir-lsp-server/completion.test
+++ mlir/test/mlir-lsp-server/completion.test
@@ -84,18 +84,18 @@
 // CHECK-NEXT:"isIncomplete": false,
 // CHECK-NEXT:"items": [
 // CHECK-NEXT:  {
-// CHECK-NEXT:"detail": "builtin.unrealized_conversion_cast: !pdl.value",
-// CHECK-NEXT:"insertText": "cast",
-// CHECK-NEXT:"insertTextFormat": 1,
-// CHECK-NEXT:"kind": 6,
-// CHECK-NEXT:"label": "%cast"
-// CHECK-NEXT:  },
-// CHECK-NEXT:  {
 // CHECK-NEXT:"detail": "arg #0: i32",
 // CHECK-NEXT:"insertText": "arg",
 // CHECK-NEXT:"insertTextFormat": 1,
 // CHECK-NEXT:"kind": 6,
 // CHECK-NEXT:"label": "%arg"
+// CHECK-NEXT:  },
+// CHECK-NEXT:  {
+// CHECK-NEXT:"detail": "builtin.unrealized_conversion_cast: !pdl.value",
+// CHECK-NEXT:"insertText": "cast",
+// CHECK-NEXT:"insertTextFormat": 1,
+// CHECK-NEXT:"kind": 6,
+// CHECK-NEXT:"label": "%cast"
 // CHECK-NEXT:  }
 // CHECK: ]
 // CHECK-NEXT:  }
Index: mlir/test/Transforms/print-op-graph.mlir
===
--- mlir/test/Transforms/print-op-graph.mlir
+++ mlir/test/Transforms/print-op-graph.mlir
@@ -6,49 +6,49 @@
 //   DFG: subgraph {{.*}}
 //   DFG:   label = "func.func{{.*}}merge_blocks
 //   DFG:   subgraph {{.*}} {
-//   DFG: v[[ARG0:.*]] [label = "arg0"
-//   DFG: v[[CONST10:.*]] [label ={{.*}}10 : i32
+//   DFG: v[[ARG0:.*]] [shape = ellipse, label = "arg0"
+//   DFG: v[[CONST10:.*]] [shape = ellipse, label ={{.*}}10 : i32
 //   DFG: subgraph [[CLUSTER_MERGE_BLOCKS:.*]] {
-//   DFG:   v[[ANCHOR:.*]] [label = " ", shape = plain]
+//   DFG:   v[[ANCHOR:.*]] [shape = plain, label = " "]
 //   DFG:   label = "test.merge_blocks
 //   DFG:   subgraph {{.*}} {
-//   DFG: v[[TEST_BR:.*]] [label = "test.br
+//   DFG: v[[TEST_BR:.*]] [shape = ellipse, label = "test.br
 //   DFG:   }
 //   DFG:   subgraph {{.*}} {
 //   DFG:   }
 //   DFG:  

[PATCH] D142862: [Support] change StringMap hash function from djbHash to xxHash

[Erik Desjardins via Phabricator via cfe-commits]

erikdesjardins added inline comments.



Comment at: llvm/test/DebugInfo/X86/gnu-public-names.ll:68-71
 ; ASM: .byte   32  # Attributes: VARIABLE, EXTERNAL
+; ASM-NEXT: .asciz  "ns::global_namespace_variable"   # External Name
+; ASM: .byte   32  # Attributes: VARIABLE, EXTERNAL
 ; ASM-NEXT: .asciz  "global_variable"   # External Name

Previously, this was

```
; ASM: .byte   32  # Attributes: VARIABLE, EXTERNAL
; ASM-NEXT: .asciz  "global_variable"   # External Name
...more lines...
; ASM: .byte   32  # Attributes: VARIABLE, EXTERNAL
; ASM-NEXT: .asciz  "ns::global_namespace_variable"   # External Name
```

but the reordering resulted in there being another `.byte   32` (a very fragile 
check line!) before `global_variable`.



Comment at: llvm/test/DebugInfo/X86/gnu-public-names.ll:189
 
-; CHECK:   DW_TAG_subprogram
+; CHECK:   [[GLOBAL_F7:0x[0-9a-f]+]]: DW_TAG_subprogram
+; CHECK: DW_AT_linkage_name

Similarly here--`f7` used to be at the end of the list below, and was just 
ignored, but now it appears in the middle


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142862

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

[PATCH] D141133: [clang-tidy] Implement CppCoreGuideline F.54

[Chris Cotter via Phabricator via cfe-commits]

ccotter added a comment.

Gentle poke for any last reviews?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141133

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

[PATCH] D142836: [clangd] Add symbol mappings for `std::experimental::filesystem`

[Younan Zhang via Phabricator via cfe-commits]

zyounan updated this revision to Diff 493195.
zyounan added a comment.

Format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142836

Files:
  clang-tools-extra/clangd/index/CanonicalIncludes.cpp
  clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
  clang/include/clang/Tooling/Inclusions/TsStdSymbolMap.inc

Index: clang/include/clang/Tooling/Inclusions/TsStdSymbolMap.inc
===
--- /dev/null
+++ clang/include/clang/Tooling/Inclusions/TsStdSymbolMap.inc
@@ -0,0 +1,58 @@
+// FIXME: Add a parser for experimental pages to
+// `clang/tools/include-mapping/cppreference_parser.py`
+
+// clang-format off
+SYMBOL(absolute, std::experimental::filesystem::, )
+SYMBOL(canonical, std::experimental::filesystem::, )
+SYMBOL(copy, std::experimental::filesystem::, )
+SYMBOL(copy_file, std::experimental::filesystem::, )
+SYMBOL(copy_options, std::experimental::filesystem::, )
+SYMBOL(copy_symlink, std::experimental::filesystem::, )
+SYMBOL(create_directories, std::experimental::filesystem::, )
+SYMBOL(create_directory, std::experimental::filesystem::, )
+SYMBOL(create_directory_symlink, std::experimental::filesystem::, )
+SYMBOL(create_hard_link, std::experimental::filesystem::, )
+SYMBOL(create_symlink, std::experimental::filesystem::, )
+SYMBOL(current_path, std::experimental::filesystem::, )
+SYMBOL(directory_entry, std::experimental::filesystem::, )
+SYMBOL(directory_iterator, std::experimental::filesystem::, )
+SYMBOL(directory_options, std::experimental::filesystem::, )
+SYMBOL(equivalent, std::experimental::filesystem::, )
+SYMBOL(exists, std::experimental::filesystem::, )
+SYMBOL(file_size, std::experimental::filesystem::, )
+SYMBOL(file_status, std::experimental::filesystem::, )
+SYMBOL(file_time_type, std::experimental::filesystem::, )
+SYMBOL(file_type, std::experimental::filesystem::, )
+SYMBOL(filesystem_error, std::experimental::filesystem::, )
+SYMBOL(hard_link_count, std::experimental::filesystem::, )
+SYMBOL(is_block_file, std::experimental::filesystem::, )
+SYMBOL(is_character_file, std::experimental::filesystem::, )
+SYMBOL(is_directory, std::experimental::filesystem::, )
+SYMBOL(is_empty, std::experimental::filesystem::, )
+SYMBOL(is_fifo, std::experimental::filesystem::, )
+SYMBOL(is_other, std::experimental::filesystem::, )
+SYMBOL(is_regular_file, std::experimental::filesystem::, )
+SYMBOL(is_socket, std::experimental::filesystem::, )
+SYMBOL(is_symlink, std::experimental::filesystem::, )
+SYMBOL(last_write_time, std::experimental::filesystem::, )
+SYMBOL(path, std::experimental::filesystem::, )
+SYMBOL(perm_options, std::experimental::filesystem::, )
+SYMBOL(permissions, std::experimental::filesystem::, )
+SYMBOL(perms, std::experimental::filesystem::, )
+SYMBOL(proximate, std::experimental::filesystem::, )
+SYMBOL(read_symlink, std::experimental::filesystem::, )
+SYMBOL(recursive_directory_iterator, std::experimental::filesystem::, )
+SYMBOL(relative, std::experimental::filesystem::, )
+SYMBOL(remove, std::experimental::filesystem::, )
+SYMBOL(remove_all, std::experimental::filesystem::, )
+SYMBOL(rename, std::experimental::filesystem::, )
+SYMBOL(resize_file, std::experimental::filesystem::, )
+SYMBOL(space, std::experimental::filesystem::, )
+SYMBOL(space_info, std::experimental::filesystem::, )
+SYMBOL(status, std::experimental::filesystem::, )
+SYMBOL(status_known, std::experimental::filesystem::, )
+SYMBOL(symlink_status, std::experimental::filesystem::, )
+SYMBOL(temp_directory_path, std::experimental::filesystem::, )
+SYMBOL(u8path, std::experimental::filesystem::, )
+SYMBOL(weakly_canonical, std::experimental::filesystem::, )
+// clang-format on
Index: clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
===
--- clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
+++ clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
@@ -62,6 +62,23 @@
   EXPECT_EQ("", CI.mapHeader(File));
 }
 
+TEST(CanonicalIncludesTest, CXXSymbolsFromTS) {
+  CanonicalIncludes CI;
+  auto Language = LangOptions();
+  Language.CPlusPlus = true;
+  CI.addSystemHeadersMapping(Language);
+
+  EXPECT_EQ("",
+CI.mapSymbol("std::experimental::filesystem::path"));
+  EXPECT_EQ("",
+CI.mapSymbol("std::experimental::filesystem::file_type"));
+  EXPECT_EQ("",
+CI.mapSymbol("std::experimental::filesystem::file_status"));
+  EXPECT_EQ("",
+CI.mapSymbol(
+"std::experimental::filesystem::recursive_directory_iterator"));
+}
+
 TEST(CanonicalIncludesTest, PathMapping) {
   auto InMemFS = llvm::makeIntrusiveRefCnt();
   FileManager Files(FileSystemOptions(), InMemFS);
Index: clang-tools-extra/clangd/index/CanonicalIncludes.cpp
===
--- clang-tools-extra/clangd/index

[PATCH] D142804: [clang-format] Support clang-format on/off line comments as prefixes

[Alex via Phabricator via cfe-commits]

alexolog added inline comments.



Comment at: clang/lib/Format/Format.cpp:3893
 
+bool isClangFormatOn(StringRef Comment) {
+  if (Comment == "/* clang-format on */")

owenpan wrote:
> alexolog wrote:
> > alexolog wrote:
> > > Here's my attempt at something flexible:
> > > Disclaimer: this is my first time looking at the LLVM codebase, so don't 
> > > expect production quality.
> > > 
> > > ```
> > > 
> > > 
> > > // Implementation detail, hide in a namespace and/or take out of the 
> > > header
> > > bool isClangFormatMarked(StringRef Comment, StringRef Mark) {
> > >   // Quick heuristics: minimum length and starts with a slash (comment)
> > >   // Shortest tag: "//clang-format on", 17 characters
> > >   static constexpr StringLiteral clangFormatStr("clang-format ");
> > >   if (Comment.size() < clangFormatStr.size() + 4 || Comment[0] != '/')
> > > return false;
> > > 
> > >   // check if it's a comment starting with "//" or "/*"
> > >   bool CloseNeeded = false;
> > >   if (Comment[1] == '*')
> > > CloseNeeded = true;
> > >   else if (Comment[1] != '/')
> > > return false;
> > > 
> > >   // Remove the comment start and all following whitespace
> > >   Comment = Comment.substr(2).ltrim();
> > > 
> > >   // Check for the actual command, a piece at a time
> > >   if (!Comment.consume_front(clangFormatStr) || 
> > > !Comment.consume_front(Mark))
> > > return false;
> > > 
> > >   // Are we there yet?
> > >   if (!CloseNeeded && Comment.empty() ||
> > >   CloseNeeded && Comment.starts_with("*/"))
> > > return true;
> > > 
> > >   // For a trailer, restrict the next character
> > >   // (currently spaces and tabs, but can include a colon, etc.)
> > >   static constexpr StringLiteral Separator(" \t");
> > >   if (!Separator.contains(Comment[0]))
> > > return false;
> > >   
> > >   // Verify comment is properly terminated
> > >   if (!CloseNeeded || Comment.contains("*/"))
> > > return true;
> > > 
> > >   return false; // Everything else
> > > }
> > > 
> > > 
> > > 
> > > bool isClangFormatOn(StringRef Comment) {
> > >   return isClangFormatMarked(Comment, "on");
> > > }
> > > 
> > > bool isClangFormatOff(StringRef Comment) {
> > >   return isClangFormatMarked(Comment, "off");
> > > }
> > > 
> > > 
> > > 
> > >   EXPECT_TRUE(isClangFormatOn("//clang-format on"));
> > >   EXPECT_TRUE(isClangFormatOn("// clang-format on"));
> > >   EXPECT_TRUE(isClangFormatOn("//clang-format on "));
> > >   EXPECT_TRUE(isClangFormatOn("//clang-format on and off"));
> > >   EXPECT_TRUE(isClangFormatOn("/*clang-format on*/"));
> > >   EXPECT_TRUE(isClangFormatOn("/* clang-format on*/"));
> > >   EXPECT_TRUE(isClangFormatOn("/*clang-format on */"));
> > >   EXPECT_TRUE(isClangFormatOn("/*clang-format on*/int i{0};"));
> > > 
> > >   EXPECT_FALSE(isClangFormatOn("//clang-format  on"));
> > >   EXPECT_FALSE(isClangFormatOn("//clang-format o"));
> > >   EXPECT_FALSE(isClangFormatOn("// clang-format o"));
> > >   EXPECT_FALSE(isClangFormatOn("//clang-format ontario"));
> > >   EXPECT_FALSE(isClangFormatOn("//clang-format off"));
> > >   EXPECT_FALSE(isClangFormatOn("/*clang-format onwards*/"));
> > > 
> > > 
> > > ```
> > Sorry about the "done".  My misunderstanding
> > Here's my attempt at something flexible:
> > Disclaimer: this is my first time looking at the LLVM codebase, so don't 
> > expect production quality.
> 
> Thanks! If we didn't have to worry about regressions, we might want to do 
> something like what you suggested above.
Isn't it what extensive test coverage is for?


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

https://reviews.llvm.org/D142804

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

[PATCH] D142867: [Clang] Add machinery to catch overflow in unary minus outside of a constant expression context

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik created this revision.
shafik added reviewers: erichkeane, aaron.ballman.
Herald added a project: All.
shafik requested review of this revision.

We provide several diagnostics for various undefined behaviors due to signed 
integer overflow outside of a constant expression context. We were missing the 
machinery to catch overflows due to unary minus.

Fixes: https://github.com/llvm/llvm-project/issues/31643


https://reviews.llvm.org/D142867

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CXX/expr/expr.const/p2-0x.cpp
  clang/test/SemaCXX/integer-overflow.cpp
  clang/unittests/Support/TimeProfilerTest.cpp


Index: clang/unittests/Support/TimeProfilerTest.cpp
===
--- clang/unittests/Support/TimeProfilerTest.cpp
+++ clang/unittests/Support/TimeProfilerTest.cpp
@@ -179,6 +179,7 @@
 Frontend
 | EvaluateAsRValue ()
 | EvaluateForOverflow ()
+| EvaluateForOverflow ()
 | EvaluateAsRValue ()
 | EvaluateForOverflow ()
 | isPotentialConstantExpr (slow_namespace::slow_func)
Index: clang/test/SemaCXX/integer-overflow.cpp
===
--- clang/test/SemaCXX/integer-overflow.cpp
+++ clang/test/SemaCXX/integer-overflow.cpp
@@ -208,3 +208,9 @@
 }
   }
 }
+
+namespace GH31643 {
+void f() {
+  int a = -(1<<31); // expected-warning {{overflow in expression; result is 
-2147483648 with type 'int'}}
+}
+}
Index: clang/test/CXX/expr/expr.const/p2-0x.cpp
===
--- clang/test/CXX/expr/expr.const/p2-0x.cpp
+++ clang/test/CXX/expr/expr.const/p2-0x.cpp
@@ -146,6 +146,7 @@
 
   constexpr int int_min = ~0x7fff;
   constexpr int minus_int_min = -int_min; // expected-error {{constant 
expression}} expected-note {{value 2147483648 is outside the range}}
+  constexpr int minus_int_min_from_shift = -(1<<31); // expected-error 
{{constant expression}} expected-note {{value 2147483648 is outside the range}}
   constexpr int div0 = 3 / 0; // expected-error {{constant expression}} 
expected-note {{division by zero}}
   constexpr int mod0 = 3 % 0; // expected-error {{constant expression}} 
expected-note {{division by zero}}
   constexpr int int_min_div_minus_1 = int_min / -1; // expected-error 
{{constant expression}} expected-note {{value 2147483648 is outside the range}}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -14853,7 +14853,7 @@
 Expr *OriginalE = Exprs.pop_back_val();
 Expr *E = OriginalE->IgnoreParenCasts();
 
-if (isa(E)) {
+if (isa(E)) {
   E->EvaluateForOverflow(Context);
   continue;
 }
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -13500,10 +13500,17 @@
   return false;
 if (!Result.isInt()) return Error(E);
 const APSInt &Value = Result.getInt();
-if (Value.isSigned() && Value.isMinSignedValue() && E->canOverflow() &&
-!HandleOverflow(Info, E, -Value.extend(Value.getBitWidth() + 1),
-E->getType()))
-  return false;
+if (Value.isSigned() && Value.isMinSignedValue() && E->canOverflow()) {
+
+  if (Info.checkingForUndefinedBehavior())
+Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
+ diag::warn_integer_constant_overflow)
+<< toString(Value, 10) << E->getType();
+
+  if (!HandleOverflow(Info, E, -Value.extend(Value.getBitWidth() + 1),
+  E->getType()))
+return false;
+}
 return Success(-Value, E);
   }
   case UO_Not: {


Index: clang/unittests/Support/TimeProfilerTest.cpp
===
--- clang/unittests/Support/TimeProfilerTest.cpp
+++ clang/unittests/Support/TimeProfilerTest.cpp
@@ -179,6 +179,7 @@
 Frontend
 | EvaluateAsRValue ()
 | EvaluateForOverflow ()
+| EvaluateForOverflow ()
 | EvaluateAsRValue ()
 | EvaluateForOverflow ()
 | isPotentialConstantExpr (slow_namespace::slow_func)
Index: clang/test/SemaCXX/integer-overflow.cpp
===
--- clang/test/SemaCXX/integer-overflow.cpp
+++ clang/test/SemaCXX/integer-overflow.cpp
@@ -208,3 +208,9 @@
 }
   }
 }
+
+namespace GH31643 {
+void f() {
+  int a = -(1<<31); // expected-warning {{overflow in expression; result is -2147483648 with type 'int'}}
+}
+}
Index: clang/test/CXX/expr/expr.const/p2-0x.cpp
===
--- clang/test/CXX/expr/expr.const/p2-0x.cpp
+++ clang/test/CXX/expr/expr.const/p2-0x.cpp
@@ -146,6 +146,7 @@
 
   constexpr int int_min = ~0x7fff;
   constexpr int minus_int_min = -int_min; // expected-error {{

[PATCH] D142440: [clangd] Don't show 'auto' type hint when type deduction fails

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 493203.
nridge added a comment.

address nit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142440

Files:
  clang-tools-extra/clangd/InlayHints.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp


Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1256,6 +1256,13 @@
   )cpp");
 }
 
+TEST(TypeHints, InvalidType) {
+  assertTypeHints(R"cpp(
+auto x = (unknown_type)42; /*error-ok*/
+auto *y = (unknown_ptr)nullptr;
+  )cpp");
+}
+
 TEST(TypeHints, ReturnTypeDeduction) {
   assertTypeHints(
   R"cpp(
Index: clang-tools-extra/clangd/InlayHints.cpp
===
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -303,8 +303,8 @@
   return true;
 }
 
-if (D->getType()->getContainedAutoType()) {
-  if (!D->getType()->isDependentType()) {
+if (auto *AT = D->getType()->getContainedAutoType()) {
+  if (AT->isDeduced() && !D->getType()->isDependentType()) {
 // Our current approach is to place the hint on the variable
 // and accordingly print the full type
 // (e.g. for `const auto& x = 42`, print `const int&`).


Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1256,6 +1256,13 @@
   )cpp");
 }
 
+TEST(TypeHints, InvalidType) {
+  assertTypeHints(R"cpp(
+auto x = (unknown_type)42; /*error-ok*/
+auto *y = (unknown_ptr)nullptr;
+  )cpp");
+}
+
 TEST(TypeHints, ReturnTypeDeduction) {
   assertTypeHints(
   R"cpp(
Index: clang-tools-extra/clangd/InlayHints.cpp
===
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -303,8 +303,8 @@
   return true;
 }
 
-if (D->getType()->getContainedAutoType()) {
-  if (!D->getType()->isDependentType()) {
+if (auto *AT = D->getType()->getContainedAutoType()) {
+  if (AT->isDeduced() && !D->getType()->isDependentType()) {
 // Our current approach is to place the hint on the variable
 // and accordingly print the full type
 // (e.g. for `const auto& x = 42`, print `const int&`).
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] 68d7f69 - [clangd] Don't show 'auto' type hint when type deduction fails

[Nathan Ridge via cfe-commits]

Author: Nathan Ridge
Date: 2023-01-30T01:21:09-05:00
New Revision: 68d7f69001c5123d3ec4a93df483a417d981d415

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

LOG: [clangd] Don't show 'auto' type hint when type deduction fails

Fixes https://github.com/clangd/clangd/issues/1475

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

Added: 


Modified: 
clang-tools-extra/clangd/InlayHints.cpp
clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/InlayHints.cpp 
b/clang-tools-extra/clangd/InlayHints.cpp
index f4e38aeb653ec..a009f84d448ae 100644
--- a/clang-tools-extra/clangd/InlayHints.cpp
+++ b/clang-tools-extra/clangd/InlayHints.cpp
@@ -304,8 +304,8 @@ class InlayHintVisitor : public 
RecursiveASTVisitor {
   return true;
 }
 
-if (D->getType()->getContainedAutoType()) {
-  if (!D->getType()->isDependentType()) {
+if (auto *AT = D->getType()->getContainedAutoType()) {
+  if (AT->isDeduced() && !D->getType()->isDependentType()) {
 // Our current approach is to place the hint on the variable
 // and accordingly print the full type
 // (e.g. for `const auto& x = 42`, print `const int&`).

diff  --git a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp 
b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
index 06f2c055a4d3c..459c51a44d575 100644
--- a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1256,6 +1256,13 @@ TEST(TypeHints, StructuredBindings_NoInitializer) {
   )cpp");
 }
 
+TEST(TypeHints, InvalidType) {
+  assertTypeHints(R"cpp(
+auto x = (unknown_type)42; /*error-ok*/
+auto *y = (unknown_ptr)nullptr;
+  )cpp");
+}
+
 TEST(TypeHints, ReturnTypeDeduction) {
   assertTypeHints(
   R"cpp(



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

[PATCH] D142440: [clangd] Don't show 'auto' type hint when type deduction fails

[Nathan Ridge 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 rG68d7f69001c5: [clangd] Don't show 'auto' type 
hint when type deduction fails (authored by nridge).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142440

Files:
  clang-tools-extra/clangd/InlayHints.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp


Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1256,6 +1256,13 @@
   )cpp");
 }
 
+TEST(TypeHints, InvalidType) {
+  assertTypeHints(R"cpp(
+auto x = (unknown_type)42; /*error-ok*/
+auto *y = (unknown_ptr)nullptr;
+  )cpp");
+}
+
 TEST(TypeHints, ReturnTypeDeduction) {
   assertTypeHints(
   R"cpp(
Index: clang-tools-extra/clangd/InlayHints.cpp
===
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -304,8 +304,8 @@
   return true;
 }
 
-if (D->getType()->getContainedAutoType()) {
-  if (!D->getType()->isDependentType()) {
+if (auto *AT = D->getType()->getContainedAutoType()) {
+  if (AT->isDeduced() && !D->getType()->isDependentType()) {
 // Our current approach is to place the hint on the variable
 // and accordingly print the full type
 // (e.g. for `const auto& x = 42`, print `const int&`).


Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1256,6 +1256,13 @@
   )cpp");
 }
 
+TEST(TypeHints, InvalidType) {
+  assertTypeHints(R"cpp(
+auto x = (unknown_type)42; /*error-ok*/
+auto *y = (unknown_ptr)nullptr;
+  )cpp");
+}
+
 TEST(TypeHints, ReturnTypeDeduction) {
   assertTypeHints(
   R"cpp(
Index: clang-tools-extra/clangd/InlayHints.cpp
===
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -304,8 +304,8 @@
   return true;
 }
 
-if (D->getType()->getContainedAutoType()) {
-  if (!D->getType()->isDependentType()) {
+if (auto *AT = D->getType()->getContainedAutoType()) {
+  if (AT->isDeduced() && !D->getType()->isDependentType()) {
 // Our current approach is to place the hint on the variable
 // and accordingly print the full type
 // (e.g. for `const auto& x = 42`, print `const int&`).
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142836: [clangd] Add symbol mappings for `std::experimental::filesystem`

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a subscriber: VitaNuo.
kadircet added a comment.

thanks a lot for the patch, we're already having some efforts right now to 
improve our cppreference parsing and get to a more complete set of symbols 
(@VitaNuo for visibility, who drives these efforts).

Regarding the experimental symbols, first of all I am not sure if it's a good 
idea to include them in clangd (or other clang-tools) at all. As a clang-tool 
binary can stay around for years (e.g. people can use 3 years old clangd) and 
symbols under `std::experimental` are subject to change throughout that period, 
hence the information provided is going to be vastly different than what the 
truth is in that time window (also i don't know if they'll be "same" across 
libc++ vs libstdc++ implementations).
In addition to that, AFAICT there's no great index page to parse for 
technical-specs hence we might indeed be forced to manually curate these 
symbols and that's something we've been trying to avoid due to maintenance 
costs. In the case of technical-specs this cost will increase, because they're 
subject to change a lot more frequent than regular std symbols and value is 
even less as these are not used by majority of the developers.

So I am afraid this is not some extra complexity we can take in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142836

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

[PATCH] D142836: [clangd] Add symbol mappings for `std::experimental::filesystem`

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

And to follow up, I definitely see the problem you're facing and it's something 
we'd like to address as well, just not in the way you proposed.

This falls under the bucket of "we might have symbols missing from our stdlib 
mappings and should try to compensate for that", e.g. we should probably try 
disabling include-insertion for such cases where we have a high confidence that 
the symbol is part of stdlib but we don't have an exact mapping in our symbol 
database to remedy the issue around inserting "wrong" includes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142836

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

[PATCH] D142085: [1/3][Clang][RISCV] Add `__riscv_` prefix for vread, vwrite, vlenb, vsetvl, and vsetvlmax

[Kito Cheng via Phabricator via cfe-commits]

kito-cheng accepted this revision.
kito-cheng added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142085

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

[PATCH] D142644: [2/3][Clang][RISCV] Add `__riscv_` for non-overloaded intrinsics

[Kito Cheng via Phabricator via cfe-commits]

kito-cheng accepted this revision.
kito-cheng added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142644

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

[PATCH] D142697: [3/3][Clang][RISCV] Add `__riscv_` for overloaded intrinsics

[Kito Cheng via Phabricator via cfe-commits]

kito-cheng accepted this revision.
kito-cheng added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142697

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

[PATCH] D142836: [clangd] Add symbol mappings for `std::experimental::filesystem`

[Younan Zhang via Phabricator via cfe-commits]

zyounan added a comment.

Another question is that file `clang/Tooling/Inclusions/StdSymbolMap.inc` seems 
to be out-of-date. What's worse, the archive of cppref doesn't always update on 
time 
The latest version from official is published in 2019, while the unofficial 
version is 2022-06.
That means, symbols from C++23, like `std::expected` will be missing from this 
file.

From the aspect of maintenance, shall we consider a more optimal approach to 
keep update?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142836

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

[clang-tools-extra] 89590cd - [clangd] Fix test failure in TypeHints.Decltype

[Nathan Ridge via cfe-commits]

Author: Nathan Ridge
Date: 2023-01-30T02:08:02-05:00
New Revision: 89590cd1fe7ce61edf0ba59d3e7dc171e5fc0211

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

LOG: [clangd] Fix test failure in TypeHints.Decltype

Added: 


Modified: 
clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp 
b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
index 459c51a44d57..da9073c77f7c 100644
--- a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1393,8 +1393,7 @@ TEST(TypeHints, Decltype) {
   ExpectedHint{": int", "a"}, ExpectedHint{": int", "b"},
   ExpectedHint{": int", "c"}, ExpectedHint{": int", "e"},
   ExpectedHint{": int", "f"}, ExpectedHint{": int", "g"},
-  ExpectedHint{": int", "h"}, ExpectedHint{": int", "i"},
-  ExpectedHint{": auto", "j"});
+  ExpectedHint{": int", "h"}, ExpectedHint{": int", "i"});
 }
 
 TEST(DesignatorHints, Basic) {



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

[PATCH] D142836: [clangd] Add symbol mappings for `std::experimental::filesystem`

[Younan Zhang via Phabricator via cfe-commits]

zyounan added a comment.

In D142836#4089721 , @kadircet wrote:

> And to follow up, I definitely see the problem you're facing and it's 
> something we'd like to address as well, just not in the way you proposed.
>
> This falls under the bucket of "we might have symbols missing from our stdlib 
> mappings and should try to compensate for that", e.g. we should probably try 
> disabling include-insertion for such cases where we have a high confidence 
> that the symbol is part of stdlib but we don't have an exact mapping in our 
> symbol database to remedy the issue around inserting "wrong" includes.

Yes, this is another issue I want to address, but you've already pointed out 
that the parser is improving.

The cppref archive page is not always up to date. As of now, the official 
version is still dumped in 2019, that means we're likely missing (partial) 
symbols from C++20/23, e.g. `std::expected`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142836

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

[PATCH] D139926: [clangd] Add semantic token for angle brackets

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D139926#4084970 , @ckandeler wrote:

> Adding AutoTypeLoc broke tons of tests, so I left it out for now.

Poked at this a bit, it looks like `AutoTypeLoc.getLAngleLoc()` is only safe to 
access if it `isConstrained()`. This is poor API (it would be better to return 
an invalid SourceLocation rather than garbage in the `!isConstrained()` case) 
but for now we can check `isConstrained()` at our call site.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139926

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

[PATCH] D141409: [SystemZ] Fix handling of vectors and their exposure of the vector ABI.

[Andreas Krebbel via Phabricator via cfe-commits]

Andreas-Krebbel added a comment.

In D141409#4083192 , @uweigand wrote:

> In D141409#4083017 , @jonpa wrote:
>
>> Review addressed - hopefully this time with a better result.
>>
>> I think the patch now does what I want it to, while I found some test cases 
>> while checking against GCC which I am a little unsure of:
>>
>> (using GCC 12.2.1, 20220901)
>
> @Andreas-Krebbel any comments on this?
>
>> GCC does *not* emit a gnu attribute for this (A wide vector should have 
>> different alignments):
>>
>>   typedef __attribute__((vector_size(32))) int v8i32;
>>   void GlobFun(v8i32 (*f)(v8i32));
>>   static v8i32 foo(v8i32 Arg) {
>> return Arg;
>>   }
>>   void fun() {
>> GlobFun(foo);
>>   }
>
> This is somewhat unclear - these are passed via "hidden" pointers that cannot 
> be discovered by user code, and I'm not sure the alignment requirements for 
> "normal" pointer apply to those.  The current ABI doc says:
>
>> Replace such an argument by a pointer to the object, or to a copy where 
>> necessary to enforce call-by-value semantics.
>
> While not explicitly stated, it would indeed be reasonable to read into that 
> an alignment requirement, i.e. that the pointer has the same requirement as a 
> "normal" pointer to the object.  But in the new ABI, that requirement is 
> explicitly stated as 8 bytes.
>
> The old behavior is not explicitly specified by any ABI doc, so we can really 
> only go after what GCC actually used to do.   And in fact GCC used to (and 
> still does!) simply place such arguments on the stack and passes a hidden 
> pointer to the stack slot - without any dynamic stack realignment, so it in 
> fact only guarantees 8 byte alignment.  So I think it is *safe* to assume 
> that using 8 bytes always is OK - and in that sense it is safe to omit the 
> GNU attribute.
>
> On the other hand, LLVM does actually align these to 32 bytes, so for LLVM 
> the vector support feature does actually constitute a change in the de-facto 
> ABI.   (Still, LLVM-generated code doesn't appear to *rely* on the extended 
> alignment in compiler-generated code anywhere - and these pointers are not 
> discoverable by user code, so alignment cannot make any difference there 
> either.   So it should still be safe.)
>
>> GCC *does* emit a gnu attribute for this (Since the arguments are passed via 
>> address and the vectors are only 8 bytes, there should be no visible ABI, 
>> or?):
>>
>>   typedef __attribute__((vector_size(8))) int v2i32;
>>   void bar(v2i32 VArr[4], v2i32 *Dst) { *Dst = VArr[3]; } 
>
> I agree, this seems unnecessary.
>
>> GCC does *not* emit it for this, but I don't understand why as the vectors 
>> are 32 bytes in size (with v4i32 the attribute is emitted as I expected):
>>
>>   typedef __attribute__((vector_size(32))) int v8i32;
>>   void bar(v8i32 VArr[2], v8i32 *Dst) { *Dst = VArr[1]; }
>
> That seems a bug, the difference in alignment is definitely visible to user 
> code here.

Indeed, the routine doing the checks in GCC does not appear handle vector 
arguments any different if they are explicitly passed as pointers. This cannot 
be correct. I'll have a look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141409

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