-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[clang-format] Add additional custom words for equivalent clang-format on/off functionality #123543
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clang-format Author: None (Shakil582) Changesresolves #43831 I'll test it a bit more tomorrow. I think I saw something weird with the --offset functionality when testing, but my additions worked for the test cases I gave it. Full diff: https://github.com/llvm/llvm-project/pull/123543.diff 1 Files Affected:
diff --git a/clang/tools/clang-format/ClangFormat.cpp b/clang/tools/clang-format/ClangFormat.cpp
index 28610052b9b74a..64af04ef4c007c 100644
--- a/clang/tools/clang-format/ClangFormat.cpp
+++ b/clang/tools/clang-format/ClangFormat.cpp
@@ -26,6 +26,12 @@
#include "llvm/Support/InitLLVM.h"
#include "llvm/Support/Process.h"
#include <fstream>
+#include <iostream>
+#include <llvm/ADT/STLExtras.h>
+#include <llvm/ADT/StringRef.h>
+#include <llvm/Frontend/OpenMP/OMPConstants.h>
+#include <llvm/MC/MCAsmMacro.h>
+#include <sys/stat.h>
using namespace llvm;
using clang::tooling::Replacements;
@@ -214,6 +220,9 @@ static cl::opt<bool> ListIgnored("list-ignored",
cl::desc("List ignored files."),
cl::cat(ClangFormatCategory), cl::Hidden);
+static SmallVector<std::string> OnWords;
+static SmallVector<std::string> OffWords;
+
namespace clang {
namespace format {
@@ -307,6 +316,72 @@ static bool fillRanges(MemoryBuffer *Code,
unsigned Length = Sources.getFileOffset(End) - Offset;
Ranges.push_back(tooling::Range(Offset, Length));
}
+
+ if (!OnWords.empty() && !OffWords.empty()) {
+ StringRef CodeRef = Code->getBuffer();
+ std::set<size_t> OnSet;
+ if (Ranges.empty())
+ OnSet.insert(0);
+ else
+ OnSet.insert(Ranges[0].getOffset());
+ std::set<size_t> OffSet;
+ if (Ranges.size() == 1 && Ranges[0].getOffset() == 0 &&
+ Ranges[0].getLength() == CodeRef.size()) {
+ Ranges.pop_back();
+ }
+
+ for (const auto &Word : OffWords) {
+ size_t TempOffset = CodeRef.find(Word);
+ while (TempOffset != StringRef::npos) {
+ if (TempOffset > *OnSet.begin())
+ OffSet.insert(TempOffset);
+ TempOffset = CodeRef.find(Word, TempOffset + 1);
+ }
+ }
+
+ for (const auto &Word : OnWords) {
+ size_t TempOffset = CodeRef.find(Word);
+ while (TempOffset != StringRef::npos) {
+ OnSet.insert(TempOffset);
+ TempOffset = CodeRef.find(Word, TempOffset + 1);
+ }
+ }
+
+ while (!OnSet.empty()) {
+ size_t Offset = OnSet.extract(OnSet.begin()).value();
+ size_t Length;
+ if (!OffSet.empty())
+ Length = OffSet.extract(OffSet.begin()).value() - Offset;
+ else
+ Length = CodeRef.size() - Offset;
+
+ // Could result in loss of data
+ tooling::Range NewRange = {static_cast<unsigned>(Offset),
+ static_cast<unsigned>(Length)};
+
+ Ranges.push_back(NewRange);
+ }
+ }
+
+ std::sort(Ranges.begin(), Ranges.end(),
+ [](const tooling::Range &a, const tooling::Range &b) {
+ return a.getOffset() + a.getLength() <
+ b.getOffset() + b.getLength();
+ });
+
+ auto start = Ranges.begin();
+ for (auto i = Ranges.begin() + 1; i != Ranges.end(); ++i) {
+ if (start->getOffset() + start->getLength() >= i->getOffset()) {
+ tooling::Range Temp(std::min(start->getOffset(), i->getOffset()),
+ std::max(start->getLength(), i->getLength()));
+ Ranges.erase(start, i + 1);
+ start = Ranges.insert(start, Temp);
+ i = start;
+ } else {
+ ++start;
+ }
+ }
+
return false;
}
@@ -646,6 +721,15 @@ static bool isIgnored(StringRef FilePath) {
const auto Pathname{convert_to_slash(AbsPath)};
for (const auto &Pat : Patterns) {
+
+ if (Pat.slice(0, 3).equals_insensitive("on:")) {
+ OnWords.push_back(Pat.slice(3, Pat.size()).trim().str());
+ continue;
+ }
+ if (Pat.slice(0, 4).equals_insensitive("off:")) {
+ OffWords.push_back(Pat.slice(4, Pat.size()).trim().str());
+ continue;
+ }
const bool IsNegated = Pat[0] == '!';
StringRef Pattern{Pat};
if (IsNegated)
|
|
|
|
Before we could consider this your code would need unit tests and changes to the documentation to accompany the change. |
mydeveloperday
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your are missing the config part from what I can tell, also unit test and documentation
| #include "llvm/Support/InitLLVM.h" | ||
| #include "llvm/Support/Process.h" | ||
| #include <fstream> | ||
| #include <iostream> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need iostream? did you have some std::cerr << debug output that you've now removed but you've left the header?
| #include <iostream> | ||
| #include <llvm/ADT/STLExtras.h> | ||
| #include <llvm/ADT/StringRef.h> | ||
| #include <llvm/Frontend/OpenMP/OMPConstants.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need OpenMP constants here?
mydeveloperday
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why can't the OnWords/Offwords be treated identically to how //clang-format off/on is handled.
HazardyKnusperkeks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't do that in ClangFormat.cpp, because tools which use libformat directly would suddenly behave different as clang-format.
resolves #43831
I'll test it a bit more tomorrow. I think I saw something weird with the --offset functionality when testing, but my additions worked for the test cases I gave it.