[Lldb-commits] [PATCH] D91103: [tooling] Add support for fixits that indicate code will need reformatting

[Nathan James via Phabricator via lldb-commits]

njames93 created this revision.
njames93 added reviewers: rsmith, aaron.ballman, klimek, alexfh.
Herald added subscribers: lldb-commits, cfe-commits, dexonsmith.
Herald added projects: clang, LLDB.
njames93 requested review of this revision.
Herald added a subscriber: JDevlieghere.

Implementing a FixItHint which indicates that code needs reformatting but not 
changing.

This is achieved by creating a FixItHint where the RemoveRange is equal to the 
InsertFromRange.
This means any dependent clients wont need updating as that is essentially a 
no-op. For clients that (are updated to) handle it, it signifies that code 
needs reformatting.
Currently this has been added to clang-tidy but for any refactoring tool that 
uses the Replacements its rather trivial to include support.
Generally this should be tacked onto other fix-its that maybe span multiple 
lines or insert/remove braces.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91103

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang/include/clang/Basic/Diagnostic.h
  clang/include/clang/Basic/SourceLocation.h
  clang/include/clang/Tooling/Core/Replacement.h
  clang/lib/Frontend/DiagnosticRenderer.cpp
  clang/lib/Frontend/Rewrite/FixItRewriter.cpp
  clang/lib/Tooling/Core/Replacement.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -1167,9 +1167,10 @@
   // This is cobbed from clang::Rewrite::FixItRewriter.
   if (fixit.CodeToInsert.empty()) {
 if (fixit.InsertFromRange.isValid()) {
-  commit.insertFromRange(fixit.RemoveRange.getBegin(),
- fixit.InsertFromRange, /*afterToken=*/false,
- fixit.BeforePreviousInsertions);
+  if (fixit.InsertFromRange != fixit.RemoveRange)
+commit.insertFromRange(fixit.RemoveRange.getBegin(),
+   fixit.InsertFromRange, /*afterToken=*/false,
+   fixit.BeforePreviousInsertions);
   return;
 }
 commit.remove(fixit.RemoveRange);
Index: clang/lib/Tooling/Core/Replacement.cpp
===
--- clang/lib/Tooling/Core/Replacement.cpp
+++ clang/lib/Tooling/Core/Replacement.cpp
@@ -22,6 +22,7 @@
 #include "clang/Rewrite/Core/RewriteBuffer.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
@@ -42,32 +43,57 @@
 
 static const char * const InvalidLocation = "";
 
-Replacement::Replacement() : FilePath(InvalidLocation) {}
+FileRange::FileRange() : FilePath(InvalidLocation), RangeInFile(0, 0) {}
+
+FileRange::FileRange(StringRef FilePath, Range RangeInFile)
+: FilePath(FilePath), RangeInFile(RangeInFile) {}
+FileRange::FileRange(StringRef FilePath, unsigned Offset, unsigned Length)
+: FileRange(FilePath, Range(Offset, Length)) {}
+
+FileRange::FileRange(const SourceManager &Sources, SourceLocation Start,
+ unsigned Length) {
+  setFromSourceLocation(Sources, Start, Length);
+}
+
+FileRange::FileRange(const SourceManager &Sources, const CharSourceRange &Range,
+ const LangOptions &LangOpts) {
+  setFromSourceRange(Sources, Range, LangOpts);
+}
+
+bool FileRange::isValid() const { return FilePath != InvalidLocation; }
+
+void FileRange::setFromSourceLocation(const SourceManager &Sources,
+  SourceLocation Start, unsigned Length) {
+  const std::pair DecomposedLocation =
+  Sources.getDecomposedLoc(Start);
+  const FileEntry *Entry = Sources.getFileEntryForID(DecomposedLocation.first);
+  this->FilePath = std::string(Entry ? Entry->getName() : InvalidLocation);
+  this->RangeInFile = {DecomposedLocation.second, Length};
+}
+
+Replacement::Replacement() : ReplacementRange() {}
+
+Replacement::Replacement(FileRange FileRange, StringRef ReplacementText)
+: ReplacementRange(FileRange), ReplacementText(ReplacementText) {}
 
 Replacement::Replacement(StringRef FilePath, unsigned Offset, unsigned Length,
  StringRef ReplacementText)
-: FilePath(std::string(FilePath)), ReplacementRange(Offset, Length),
-  ReplacementText(std::string(ReplacementText)) {}
+: Replacement(FileRange(FilePath, Offset, Length), ReplacementText) {}
 
 Replacement::Replacement(const SourceManager &Sources, SourceLocation Start,
- unsigned Length, StringRef ReplacementText) {
-  setFromSourceLocation(Sources, Start, Length, ReplacementText);
-}
+  

[Lldb-commits] [PATCH] D91103: [tooling] Add support for fixits that indicate code will need reformatting

[Nathan James via Phabricator via lldb-commits]

njames93 added a comment.

In D91103#2384048 , @jingham wrote:

> IIUC, the expression parser part of this change suppresses any Fixits that 
> are clang-tidy type rewrites, is that right?  If so that is indeed the 
> correct behavior.  But the fact that this change implements that behavior is 
> entirely non-obvious at the call site.  Could you either add a comment here 
> explaining to point of the test or maybe wrap the test in a method on the 
> FixItHint class so that it's self documenting?

Hmm thats a good point, maybe a method inside FixItHint like,

  bool isReformatRange() const {
return InsertFromRange() == RemoveRange();
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91103

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

[Lldb-commits] [PATCH] D91103: [tooling] Add support for fixits that indicate code will need reformatting

[Nathan James via Phabricator via lldb-commits]

njames93 updated this revision to Diff 304126.
njames93 added a comment.

Add `isReformatFixit` method to `FixItHint` to better express intent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91103

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang/include/clang/Basic/Diagnostic.h
  clang/include/clang/Basic/SourceLocation.h
  clang/include/clang/Tooling/Core/Replacement.h
  clang/lib/Frontend/DiagnosticRenderer.cpp
  clang/lib/Frontend/Rewrite/FixItRewriter.cpp
  clang/lib/Tooling/Core/Replacement.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -1167,9 +1167,10 @@
   // This is cobbed from clang::Rewrite::FixItRewriter.
   if (fixit.CodeToInsert.empty()) {
 if (fixit.InsertFromRange.isValid()) {
-  commit.insertFromRange(fixit.RemoveRange.getBegin(),
- fixit.InsertFromRange, /*afterToken=*/false,
- fixit.BeforePreviousInsertions);
+  if (!fixit.isReformatFixit())
+commit.insertFromRange(fixit.RemoveRange.getBegin(),
+   fixit.InsertFromRange, /*afterToken=*/false,
+   fixit.BeforePreviousInsertions);
   return;
 }
 commit.remove(fixit.RemoveRange);
Index: clang/lib/Tooling/Core/Replacement.cpp
===
--- clang/lib/Tooling/Core/Replacement.cpp
+++ clang/lib/Tooling/Core/Replacement.cpp
@@ -22,6 +22,7 @@
 #include "clang/Rewrite/Core/RewriteBuffer.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
@@ -42,32 +43,57 @@
 
 static const char * const InvalidLocation = "";
 
-Replacement::Replacement() : FilePath(InvalidLocation) {}
+FileRange::FileRange() : FilePath(InvalidLocation), RangeInFile(0, 0) {}
+
+FileRange::FileRange(StringRef FilePath, Range RangeInFile)
+: FilePath(FilePath), RangeInFile(RangeInFile) {}
+FileRange::FileRange(StringRef FilePath, unsigned Offset, unsigned Length)
+: FileRange(FilePath, Range(Offset, Length)) {}
+
+FileRange::FileRange(const SourceManager &Sources, SourceLocation Start,
+ unsigned Length) {
+  setFromSourceLocation(Sources, Start, Length);
+}
+
+FileRange::FileRange(const SourceManager &Sources, const CharSourceRange &Range,
+ const LangOptions &LangOpts) {
+  setFromSourceRange(Sources, Range, LangOpts);
+}
+
+bool FileRange::isValid() const { return FilePath != InvalidLocation; }
+
+void FileRange::setFromSourceLocation(const SourceManager &Sources,
+  SourceLocation Start, unsigned Length) {
+  const std::pair DecomposedLocation =
+  Sources.getDecomposedLoc(Start);
+  const FileEntry *Entry = Sources.getFileEntryForID(DecomposedLocation.first);
+  this->FilePath = std::string(Entry ? Entry->getName() : InvalidLocation);
+  this->RangeInFile = {DecomposedLocation.second, Length};
+}
+
+Replacement::Replacement() : ReplacementRange() {}
+
+Replacement::Replacement(FileRange FileRange, StringRef ReplacementText)
+: ReplacementRange(FileRange), ReplacementText(ReplacementText) {}
 
 Replacement::Replacement(StringRef FilePath, unsigned Offset, unsigned Length,
  StringRef ReplacementText)
-: FilePath(std::string(FilePath)), ReplacementRange(Offset, Length),
-  ReplacementText(std::string(ReplacementText)) {}
+: Replacement(FileRange(FilePath, Offset, Length), ReplacementText) {}
 
 Replacement::Replacement(const SourceManager &Sources, SourceLocation Start,
- unsigned Length, StringRef ReplacementText) {
-  setFromSourceLocation(Sources, Start, Length, ReplacementText);
-}
+ unsigned Length, StringRef ReplacementText)
+: Replacement(FileRange(Sources, Start, Length), ReplacementText) {}
 
 Replacement::Replacement(const SourceManager &Sources,
  const CharSourceRange &Range,
- StringRef ReplacementText,
- const LangOptions &LangOpts) {
-  setFromSourceRange(Sources, Range, ReplacementText, LangOpts);
-}
+ StringRef ReplacementText, const LangOptions &LangOpts)
+: Replacement(FileRange(Sources, Range, LangOpts), ReplacementText) {}
 
-bool Replacement::isApplicable() const {
-  return FilePath != InvalidLocation;
-}
+bool Replacement::isApplicable() const { r

[Lldb-commits] [PATCH] D91103: [tooling] Add support for fixits that indicate code will need reformatting

[Nathan James via Phabricator via lldb-commits]

njames93 added a comment.

An example of this in action is D91149 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91103

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

[Lldb-commits] [PATCH] D91103: [tooling] Add support for fixits that indicate code will need reformatting

[Nathan James via Phabricator via lldb-commits]

njames93 updated this revision to Diff 304198.
njames93 added a comment.
Herald added subscribers: usaxena95, kadircet, arphaman.

Teach clangd to ignore these fix-its.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91103

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clangd/Diagnostics.cpp
  clang/include/clang/Basic/Diagnostic.h
  clang/include/clang/Basic/SourceLocation.h
  clang/include/clang/Tooling/Core/Replacement.h
  clang/lib/Frontend/DiagnosticRenderer.cpp
  clang/lib/Frontend/Rewrite/FixItRewriter.cpp
  clang/lib/Tooling/Core/Replacement.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -1167,9 +1167,10 @@
   // This is cobbed from clang::Rewrite::FixItRewriter.
   if (fixit.CodeToInsert.empty()) {
 if (fixit.InsertFromRange.isValid()) {
-  commit.insertFromRange(fixit.RemoveRange.getBegin(),
- fixit.InsertFromRange, /*afterToken=*/false,
- fixit.BeforePreviousInsertions);
+  if (!fixit.isReformatFixit())
+commit.insertFromRange(fixit.RemoveRange.getBegin(),
+   fixit.InsertFromRange, /*afterToken=*/false,
+   fixit.BeforePreviousInsertions);
   return;
 }
 commit.remove(fixit.RemoveRange);
Index: clang/lib/Tooling/Core/Replacement.cpp
===
--- clang/lib/Tooling/Core/Replacement.cpp
+++ clang/lib/Tooling/Core/Replacement.cpp
@@ -22,6 +22,7 @@
 #include "clang/Rewrite/Core/RewriteBuffer.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
@@ -42,32 +43,57 @@
 
 static const char * const InvalidLocation = "";
 
-Replacement::Replacement() : FilePath(InvalidLocation) {}
+FileRange::FileRange() : FilePath(InvalidLocation), RangeInFile(0, 0) {}
+
+FileRange::FileRange(StringRef FilePath, Range RangeInFile)
+: FilePath(FilePath), RangeInFile(RangeInFile) {}
+FileRange::FileRange(StringRef FilePath, unsigned Offset, unsigned Length)
+: FileRange(FilePath, Range(Offset, Length)) {}
+
+FileRange::FileRange(const SourceManager &Sources, SourceLocation Start,
+ unsigned Length) {
+  setFromSourceLocation(Sources, Start, Length);
+}
+
+FileRange::FileRange(const SourceManager &Sources, const CharSourceRange &Range,
+ const LangOptions &LangOpts) {
+  setFromSourceRange(Sources, Range, LangOpts);
+}
+
+bool FileRange::isValid() const { return FilePath != InvalidLocation; }
+
+void FileRange::setFromSourceLocation(const SourceManager &Sources,
+  SourceLocation Start, unsigned Length) {
+  const std::pair DecomposedLocation =
+  Sources.getDecomposedLoc(Start);
+  const FileEntry *Entry = Sources.getFileEntryForID(DecomposedLocation.first);
+  this->FilePath = std::string(Entry ? Entry->getName() : InvalidLocation);
+  this->RangeInFile = {DecomposedLocation.second, Length};
+}
+
+Replacement::Replacement() : ReplacementRange() {}
+
+Replacement::Replacement(FileRange FileRange, StringRef ReplacementText)
+: ReplacementRange(FileRange), ReplacementText(ReplacementText) {}
 
 Replacement::Replacement(StringRef FilePath, unsigned Offset, unsigned Length,
  StringRef ReplacementText)
-: FilePath(std::string(FilePath)), ReplacementRange(Offset, Length),
-  ReplacementText(std::string(ReplacementText)) {}
+: Replacement(FileRange(FilePath, Offset, Length), ReplacementText) {}
 
 Replacement::Replacement(const SourceManager &Sources, SourceLocation Start,
- unsigned Length, StringRef ReplacementText) {
-  setFromSourceLocation(Sources, Start, Length, ReplacementText);
-}
+ unsigned Length, StringRef ReplacementText)
+: Replacement(FileRange(Sources, Start, Length), ReplacementText) {}
 
 Replacement::Replacement(const SourceManager &Sources,
  const CharSourceRange &Range,
- StringRef ReplacementText,
- const LangOptions &LangOpts) {
-  setFromSourceRange(Sources, Range, ReplacementText, LangOpts);
-}
+ StringRef ReplacementText, const LangOptions &LangOpts)
+: Replacement(FileRange(Sources, Range, LangOpts), ReplacementText) {}
 
-bool Replacement::isApplicable() const {
-  return FilePat

[Lldb-commits] [PATCH] D91103: [tooling] Add support for fixits that indicate code will need reformatting

[Nathan James via Phabricator via lldb-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clangd/Diagnostics.cpp:636
+// Filter out any reformat fixits, we don't handle these.
+// FIXME: Can we?
+llvm::erase_if(FixIts,

kadircet wrote:
> in theory yes, as we have access to source manager, we can fetch file 
> contents and create formatted  replacements (see `cleanupAndFormat`). but 
> formatting those fixes can imply significant delays on clangd's diagnostic 
> cycles (if there are many of those), that's the reason why we currently don't 
> format fixits.
I mean if clangd is extended to support async code actions for diagnostics 
instead of just using the inline extension. Having said that, if that were the 
case, this would probably not be where that support is implemented.



Comment at: clang-tools-extra/clangd/Diagnostics.cpp:637
+// FIXME: Can we?
+llvm::erase_if(FixIts,
+   [](const FixItHint &Fix) { return Fix.isReformatFixit(); });

kadircet wrote:
> rather than doing an extra loop, can we just skip those in the for loop below 
> ?
We could, however that would result in messier code further down when trying to 
build a synthetic message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91103

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

[Lldb-commits] [PATCH] D91103: [tooling] Add support for fixits that indicate code will need reformatting

[Nathan James via Phabricator via lldb-commits]

njames93 updated this revision to Diff 304477.
njames93 marked 2 inline comments as done.
njames93 added a comment.

Removed the first loop for clangd diagnostic, turns out it didnt make the 
following code that much messier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91103

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clangd/Diagnostics.cpp
  clang/include/clang/Basic/Diagnostic.h
  clang/include/clang/Basic/SourceLocation.h
  clang/include/clang/Tooling/Core/Replacement.h
  clang/lib/Frontend/DiagnosticRenderer.cpp
  clang/lib/Frontend/Rewrite/FixItRewriter.cpp
  clang/lib/Tooling/Core/Replacement.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -1167,9 +1167,10 @@
   // This is cobbed from clang::Rewrite::FixItRewriter.
   if (fixit.CodeToInsert.empty()) {
 if (fixit.InsertFromRange.isValid()) {
-  commit.insertFromRange(fixit.RemoveRange.getBegin(),
- fixit.InsertFromRange, /*afterToken=*/false,
- fixit.BeforePreviousInsertions);
+  if (!fixit.isReformatFixit())
+commit.insertFromRange(fixit.RemoveRange.getBegin(),
+   fixit.InsertFromRange, /*afterToken=*/false,
+   fixit.BeforePreviousInsertions);
   return;
 }
 commit.remove(fixit.RemoveRange);
Index: clang/lib/Tooling/Core/Replacement.cpp
===
--- clang/lib/Tooling/Core/Replacement.cpp
+++ clang/lib/Tooling/Core/Replacement.cpp
@@ -22,6 +22,7 @@
 #include "clang/Rewrite/Core/RewriteBuffer.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
@@ -42,32 +43,57 @@
 
 static const char * const InvalidLocation = "";
 
-Replacement::Replacement() : FilePath(InvalidLocation) {}
+FileRange::FileRange() : FilePath(InvalidLocation), RangeInFile(0, 0) {}
+
+FileRange::FileRange(StringRef FilePath, Range RangeInFile)
+: FilePath(FilePath), RangeInFile(RangeInFile) {}
+FileRange::FileRange(StringRef FilePath, unsigned Offset, unsigned Length)
+: FileRange(FilePath, Range(Offset, Length)) {}
+
+FileRange::FileRange(const SourceManager &Sources, SourceLocation Start,
+ unsigned Length) {
+  setFromSourceLocation(Sources, Start, Length);
+}
+
+FileRange::FileRange(const SourceManager &Sources, const CharSourceRange &Range,
+ const LangOptions &LangOpts) {
+  setFromSourceRange(Sources, Range, LangOpts);
+}
+
+bool FileRange::isValid() const { return FilePath != InvalidLocation; }
+
+void FileRange::setFromSourceLocation(const SourceManager &Sources,
+  SourceLocation Start, unsigned Length) {
+  const std::pair DecomposedLocation =
+  Sources.getDecomposedLoc(Start);
+  const FileEntry *Entry = Sources.getFileEntryForID(DecomposedLocation.first);
+  this->FilePath = std::string(Entry ? Entry->getName() : InvalidLocation);
+  this->RangeInFile = {DecomposedLocation.second, Length};
+}
+
+Replacement::Replacement() : ReplacementRange() {}
+
+Replacement::Replacement(FileRange FileRange, StringRef ReplacementText)
+: ReplacementRange(FileRange), ReplacementText(ReplacementText) {}
 
 Replacement::Replacement(StringRef FilePath, unsigned Offset, unsigned Length,
  StringRef ReplacementText)
-: FilePath(std::string(FilePath)), ReplacementRange(Offset, Length),
-  ReplacementText(std::string(ReplacementText)) {}
+: Replacement(FileRange(FilePath, Offset, Length), ReplacementText) {}
 
 Replacement::Replacement(const SourceManager &Sources, SourceLocation Start,
- unsigned Length, StringRef ReplacementText) {
-  setFromSourceLocation(Sources, Start, Length, ReplacementText);
-}
+ unsigned Length, StringRef ReplacementText)
+: Replacement(FileRange(Sources, Start, Length), ReplacementText) {}
 
 Replacement::Replacement(const SourceManager &Sources,
  const CharSourceRange &Range,
- StringRef ReplacementText,
- const LangOptions &LangOpts) {
-  setFromSourceRange(Sources, Range, ReplacementText, LangOpts);
-}
+ StringRef ReplacementText, const LangOptions &LangOpts)
+: Replacement(FileRange(Sources, Range, LangOpts), ReplacementText) {}
 
-b

[Lldb-commits] [PATCH] D91103: [tooling] Add support for fixits that indicate code will need reformatting

[Nathan James via Phabricator via lldb-commits]

njames93 updated this revision to Diff 304574.
njames93 marked an inline comment as done.
njames93 added a comment.

Address nit by replacing optional.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91103

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clangd/Diagnostics.cpp
  clang/include/clang/Basic/Diagnostic.h
  clang/include/clang/Basic/SourceLocation.h
  clang/include/clang/Tooling/Core/Replacement.h
  clang/lib/Frontend/DiagnosticRenderer.cpp
  clang/lib/Frontend/Rewrite/FixItRewriter.cpp
  clang/lib/Tooling/Core/Replacement.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -1167,9 +1167,10 @@
   // This is cobbed from clang::Rewrite::FixItRewriter.
   if (fixit.CodeToInsert.empty()) {
 if (fixit.InsertFromRange.isValid()) {
-  commit.insertFromRange(fixit.RemoveRange.getBegin(),
- fixit.InsertFromRange, /*afterToken=*/false,
- fixit.BeforePreviousInsertions);
+  if (!fixit.isReformatFixit())
+commit.insertFromRange(fixit.RemoveRange.getBegin(),
+   fixit.InsertFromRange, /*afterToken=*/false,
+   fixit.BeforePreviousInsertions);
   return;
 }
 commit.remove(fixit.RemoveRange);
Index: clang/lib/Tooling/Core/Replacement.cpp
===
--- clang/lib/Tooling/Core/Replacement.cpp
+++ clang/lib/Tooling/Core/Replacement.cpp
@@ -22,6 +22,7 @@
 #include "clang/Rewrite/Core/RewriteBuffer.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
@@ -42,32 +43,57 @@
 
 static const char * const InvalidLocation = "";
 
-Replacement::Replacement() : FilePath(InvalidLocation) {}
+FileRange::FileRange() : FilePath(InvalidLocation), RangeInFile(0, 0) {}
+
+FileRange::FileRange(StringRef FilePath, Range RangeInFile)
+: FilePath(FilePath), RangeInFile(RangeInFile) {}
+FileRange::FileRange(StringRef FilePath, unsigned Offset, unsigned Length)
+: FileRange(FilePath, Range(Offset, Length)) {}
+
+FileRange::FileRange(const SourceManager &Sources, SourceLocation Start,
+ unsigned Length) {
+  setFromSourceLocation(Sources, Start, Length);
+}
+
+FileRange::FileRange(const SourceManager &Sources, const CharSourceRange &Range,
+ const LangOptions &LangOpts) {
+  setFromSourceRange(Sources, Range, LangOpts);
+}
+
+bool FileRange::isValid() const { return FilePath != InvalidLocation; }
+
+void FileRange::setFromSourceLocation(const SourceManager &Sources,
+  SourceLocation Start, unsigned Length) {
+  const std::pair DecomposedLocation =
+  Sources.getDecomposedLoc(Start);
+  const FileEntry *Entry = Sources.getFileEntryForID(DecomposedLocation.first);
+  this->FilePath = std::string(Entry ? Entry->getName() : InvalidLocation);
+  this->RangeInFile = {DecomposedLocation.second, Length};
+}
+
+Replacement::Replacement() : ReplacementRange() {}
+
+Replacement::Replacement(FileRange FileRange, StringRef ReplacementText)
+: ReplacementRange(FileRange), ReplacementText(ReplacementText) {}
 
 Replacement::Replacement(StringRef FilePath, unsigned Offset, unsigned Length,
  StringRef ReplacementText)
-: FilePath(std::string(FilePath)), ReplacementRange(Offset, Length),
-  ReplacementText(std::string(ReplacementText)) {}
+: Replacement(FileRange(FilePath, Offset, Length), ReplacementText) {}
 
 Replacement::Replacement(const SourceManager &Sources, SourceLocation Start,
- unsigned Length, StringRef ReplacementText) {
-  setFromSourceLocation(Sources, Start, Length, ReplacementText);
-}
+ unsigned Length, StringRef ReplacementText)
+: Replacement(FileRange(Sources, Start, Length), ReplacementText) {}
 
 Replacement::Replacement(const SourceManager &Sources,
  const CharSourceRange &Range,
- StringRef ReplacementText,
- const LangOptions &LangOpts) {
-  setFromSourceRange(Sources, Range, ReplacementText, LangOpts);
-}
+ StringRef ReplacementText, const LangOptions &LangOpts)
+: Replacement(FileRange(Sources, Range, LangOpts), ReplacementText) {}
 
-bool Replacement::isApplicable() const {
-  return FilePath != InvalidLocat

[Lldb-commits] [PATCH] D93733: [NFC] replace resize calls with resize_for_overwrite

[Nathan James via Phabricator via lldb-commits]

njames93 created this revision.
Herald added subscribers: dexonsmith, asbirlea, hiraditya.
njames93 published this revision for review.
Herald added projects: clang, LLDB, LLVM.
Herald added subscribers: llvm-commits, lldb-commits, cfe-commits.

Also replace a few calls to resize followed by zeroing the data to calling 
assign(size, 0).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D93733

Files:
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Lex/Lexer.cpp
  clang/lib/Lex/LiteralSupport.cpp
  clang/lib/Lex/Preprocessor.cpp
  clang/lib/Sema/SemaExpr.cpp
  lldb/source/Core/ValueObject.cpp
  lldb/source/Host/common/LZMA.cpp
  lldb/source/Utility/VASprintf.cpp
  llvm/lib/CodeGen/MachineLICM.cpp
  llvm/lib/CodeGen/TargetSchedule.cpp
  llvm/lib/DebugInfo/CodeView/TypeStreamMerger.cpp
  llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
  llvm/lib/MC/MCAsmStreamer.cpp
  llvm/lib/MC/MCDwarf.cpp
  llvm/lib/MC/MCParser/AsmParser.cpp
  llvm/lib/MC/MCParser/MasmParser.cpp
  llvm/lib/MC/MCStreamer.cpp
  llvm/lib/MC/StringTableBuilder.cpp
  llvm/lib/MC/WinCOFFObjectWriter.cpp
  llvm/lib/Object/IRSymtab.cpp
  llvm/lib/Object/MachOObjectFile.cpp
  llvm/lib/Support/ConvertUTFWrapper.cpp
  llvm/lib/Support/PrettyStackTrace.cpp
  llvm/lib/Support/raw_ostream.cpp

Index: llvm/lib/Support/raw_ostream.cpp
===
--- llvm/lib/Support/raw_ostream.cpp
+++ llvm/lib/Support/raw_ostream.cpp
@@ -331,7 +331,7 @@
   SmallVector V;
 
   while (true) {
-V.resize(NextBufferSize);
+V.resize_for_overwrite(NextBufferSize);
 
 // Try formatting into the SmallVector.
 size_t BytesUsed = Fmt.print(V.data(), NextBufferSize);
Index: llvm/lib/Support/PrettyStackTrace.cpp
===
--- llvm/lib/Support/PrettyStackTrace.cpp
+++ llvm/lib/Support/PrettyStackTrace.cpp
@@ -243,7 +243,7 @@
   }
 
   const int Size = SizeOrError + 1; // '\0'
-  Str.resize(Size);
+  Str.resize_for_overwrite(Size);
   va_start(AP, Format);
   vsnprintf(Str.data(), Size, Format, AP);
   va_end(AP);
Index: llvm/lib/Support/ConvertUTFWrapper.cpp
===
--- llvm/lib/Support/ConvertUTFWrapper.cpp
+++ llvm/lib/Support/ConvertUTFWrapper.cpp
@@ -160,7 +160,7 @@
   // UTF-8 encoding.  Allocate one extra byte for the null terminator though,
   // so that someone calling DstUTF16.data() gets a null terminated string.
   // We resize down later so we don't have to worry that this over allocates.
-  DstUTF16.resize(SrcUTF8.size()+1);
+  DstUTF16.resize_for_overwrite(SrcUTF8.size() + 1);
   UTF16 *Dst = &DstUTF16[0];
   UTF16 *DstEnd = Dst + DstUTF16.size();
 
Index: llvm/lib/Object/MachOObjectFile.cpp
===
--- llvm/lib/Object/MachOObjectFile.cpp
+++ llvm/lib/Object/MachOObjectFile.cpp
@@ -867,7 +867,7 @@
   " LC_BUILD_VERSION_COMMAND has incorrect cmdsize");
 
   auto Start = Load.Ptr + sizeof(MachO::build_version_command);
-  BuildTools.resize(BVC.ntools);
+  BuildTools.resize_for_overwrite(BVC.ntools);
   for (unsigned i = 0; i < BVC.ntools; ++i)
 BuildTools[i] = Start + i * sizeof(MachO::build_tool_version);
 
Index: llvm/lib/Object/IRSymtab.cpp
===
--- llvm/lib/Object/IRSymtab.cpp
+++ llvm/lib/Object/IRSymtab.cpp
@@ -327,7 +327,7 @@
 
   // We are about to fill in the header's range fields, so reserve space for it
   // and copy it in afterwards.
-  Symtab.resize(sizeof(storage::Header));
+  Symtab.resize_for_overwrite(sizeof(storage::Header));
   writeRange(Hdr.Modules, Mods);
   writeRange(Hdr.Comdats, Comdats);
   writeRange(Hdr.Symbols, Syms);
@@ -370,7 +370,7 @@
 return std::move(E);
 
   StrtabBuilder.finalizeInOrder();
-  FC.Strtab.resize(StrtabBuilder.getSize());
+  FC.Strtab.resize_for_overwrite(StrtabBuilder.getSize());
   StrtabBuilder.write((uint8_t *)FC.Strtab.data());
 
   FC.TheReader = {{FC.Symtab.data(), FC.Symtab.size()},
Index: llvm/lib/MC/WinCOFFObjectWriter.cpp
===
--- llvm/lib/MC/WinCOFFObjectWriter.cpp
+++ llvm/lib/MC/WinCOFFObjectWriter.cpp
@@ -860,7 +860,7 @@
 COFFSymbol *File = createSymbol(".file");
 File->Data.SectionNumber = COFF::IMAGE_SYM_DEBUG;
 File->Data.StorageClass = COFF::IMAGE_SYM_CLASS_FILE;
-File->Aux.resize(Count);
+File->Aux.resize_for_overwrite(Count);
 
 unsigned Offset = 0;
 unsigned Length = Name.size();
Index: llvm/lib/MC/StringTableBuilder.cpp
===
--- llvm/lib/MC/StringTableBuilder.cpp
+++ llvm/lib/MC/StringTableBuilder.cpp
@@ -59,7 +59,7 @@
 void StringTableBuilder::write(raw_ostream &OS) const {
   assert(isFinalized());
   SmallSt