[clang][analyzer] Add plist macro formatting #156046
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. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis Author: None (ankurkraj) ChangesFixes Issue#154743 Full diff: https://github.com/llvm/llvm-project/pull/156046.diff 5 Files Affected:
diff --git a/clang/include/clang/Analysis/PathDiagnostic.h b/clang/include/clang/Analysis/PathDiagnostic.h
index 5907df022e449..665edcf8756ad 100644
--- a/clang/include/clang/Analysis/PathDiagnostic.h
+++ b/clang/include/clang/Analysis/PathDiagnostic.h
@@ -68,6 +68,11 @@ struct PathDiagnosticConsumerOptions {
/// without re-compiling the program under analysis.
bool ShouldDisplayMacroExpansions = false;
+ /// Whether to include clang-formatted macros during macro expansions
+ /// or to keep it as it was stored, the default setting is to kee it as
+ /// is.
+ bool ShouldFormatMacrosPlist = false;
+
/// Whether to include LLVM statistics of the process in the diagnostic.
/// Useful for profiling the tool on large real-world codebases.
bool ShouldSerializeStats = false;
diff --git a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
index 90b80e5201aa8..3f19e3020ba42 100644
--- a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
+++ b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
@@ -351,6 +351,11 @@ ANALYZER_OPTION(bool, ShouldDisplayMacroExpansions, "expand-macros",
"expanded and included in the plist output.",
false)
+ANALYZER_OPTION(bool, ShouldFormatMacrosPlist, "format-macros-plist",
+ "Whether the macros being displayed in the plist "
+ "files are clang-formatted .",
+ false)
+
ANALYZER_OPTION(bool, DisplayCTUProgress, "display-ctu-progress",
"Whether to emit verbose output about "
"the analyzer's progress related to ctu.",
diff --git a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
index 7d0c2d8658f35..d8f32d659a6a1 100644
--- a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
+++ b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
@@ -403,16 +403,14 @@ class AnalyzerOptions {
bool mayInlineCXXMemberFunction(CXXInlineableMemberKind K) const;
ento::PathDiagnosticConsumerOptions getDiagOpts() const {
- return {FullCompilerInvocation,
- ShouldDisplayMacroExpansions,
- ShouldSerializeStats,
+ return {FullCompilerInvocation, ShouldDisplayMacroExpansions,
+ ShouldFormatMacrosPlist, ShouldSerializeStats,
// The stable report filename option is deprecated because
// file names are now always stable. Now the old option acts as
// an alias to the new verbose filename option because this
// closely mimics the behavior under the old option.
ShouldWriteStableReportFilename || ShouldWriteVerboseReportFilename,
- static_cast<bool>(AnalyzerWerror),
- ShouldApplyFixIts,
+ static_cast<bool>(AnalyzerWerror), ShouldApplyFixIts,
ShouldDisplayCheckerNameForText};
}
};
diff --git a/clang/lib/StaticAnalyzer/Core/CMakeLists.txt b/clang/lib/StaticAnalyzer/Core/CMakeLists.txt
index d0a9b202f9c52..6d51fcd0e7cc8 100644
--- a/clang/lib/StaticAnalyzer/Core/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Core/CMakeLists.txt
@@ -60,6 +60,7 @@ add_clang_library(clangStaticAnalyzerCore
clangAnalysis
clangBasic
clangCrossTU
+ clangFormat
clangFrontend
clangLex
clangRewrite
diff --git a/clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp b/clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
index 771d09e19f178..d38e6491c1ce3 100644
--- a/clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
+++ b/clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -17,6 +17,7 @@
#include "clang/Basic/SourceManager.h"
#include "clang/Basic/Version.h"
#include "clang/CrossTU/CrossTranslationUnit.h"
+#include "clang/Format/Format.h"
#include "clang/Frontend/ASTUnit.h"
#include "clang/Lex/Preprocessor.h"
#include "clang/Lex/TokenConcatenation.h"
@@ -100,7 +101,8 @@ class PlistPrinter {
/// is found through a call piece, etc), it's subpieces are reported, and the
/// piece itself is collected. Call this function after the entire bugpath
/// was reported.
- void ReportMacroExpansions(raw_ostream &o, unsigned indent);
+ void ReportMacroExpansions(raw_ostream &o, unsigned indent,
+ bool ShouldFormatMacrosPlist);
private:
void ReportPiece(raw_ostream &o, const PathDiagnosticPiece &P,
@@ -163,9 +165,15 @@ static void printCoverage(const PathDiagnostic *D,
FIDMap &FM,
llvm::raw_fd_ostream &o);
-static std::optional<StringRef> getExpandedMacro(
- SourceLocation MacroLoc, const cross_tu::CrossTranslationUnitContext &CTU,
- const MacroExpansionContext &MacroExpansions, const SourceManager &SM);
+static std::optional<StringRef>
+getExpandedMacro(SourceLocation MacroLoc,
+ const cross_tu::CrossTranslationUnitContext &CTU,
+ const MacroExpansionContext &MacroExpansions,
+ const SourceManager &SM, bool ShouldFormatMacrosPlist);
+
+static std::optional<StringRef>
+getFormattedMacro(std::optional<StringRef> ExpandedText,
+ bool ShouldFormatMacrosPlist);
//===----------------------------------------------------------------------===//
// Methods of PlistPrinter.
@@ -372,7 +380,8 @@ void PlistPrinter::ReportMacroSubPieces(raw_ostream &o,
"Fixits on constrol flow pieces are not implemented yet!");
}
-void PlistPrinter::ReportMacroExpansions(raw_ostream &o, unsigned indent) {
+void PlistPrinter::ReportMacroExpansions(raw_ostream &o, unsigned indent,
+ bool ShouldFormatMacrosPlist) {
for (const PathDiagnosticMacroPiece *P : MacroPieces) {
const SourceManager &SM = PP.getSourceManager();
@@ -382,8 +391,8 @@ void PlistPrinter::ReportMacroExpansions(raw_ostream &o, unsigned indent) {
const std::optional<StringRef> MacroName =
MacroExpansions.getOriginalText(MacroExpansionLoc);
- const std::optional<StringRef> ExpansionText =
- getExpandedMacro(MacroExpansionLoc, CTU, MacroExpansions, SM);
+ const std::optional<StringRef> ExpansionText = getExpandedMacro(
+ MacroExpansionLoc, CTU, MacroExpansions, SM, ShouldFormatMacrosPlist);
if (!MacroName || !ExpansionText)
continue;
@@ -602,7 +611,8 @@ void PlistDiagnostics::printBugPath(llvm::raw_ostream &o, const FIDMap &FM,
o << " <key>macro_expansions</key>\n"
" <array>\n";
- Printer.ReportMacroExpansions(o, /* indent */ 4);
+ Printer.ReportMacroExpansions(o, /* indent */ 4,
+ DiagOpts.ShouldFormatMacrosPlist);
o << " </array>\n";
}
@@ -824,10 +834,49 @@ static std::optional<StringRef>
getExpandedMacro(SourceLocation MacroExpansionLoc,
const cross_tu::CrossTranslationUnitContext &CTU,
const MacroExpansionContext &MacroExpansions,
- const SourceManager &SM) {
+ const SourceManager &SM, bool ShouldFormatMacrosPlist) {
+ std::optional<StringRef> ExpandedText;
if (auto CTUMacroExpCtx =
CTU.getMacroExpansionContextForSourceLocation(MacroExpansionLoc)) {
- return CTUMacroExpCtx->getExpandedText(MacroExpansionLoc);
+ ExpandedText = CTUMacroExpCtx->getExpandedText(MacroExpansionLoc);
+ } else {
+ ExpandedText = MacroExpansions.getExpandedText(MacroExpansionLoc);
}
- return MacroExpansions.getExpandedText(MacroExpansionLoc);
+
+ std::optional<StringRef> FormattedMacroText =
+ getFormattedMacro(ExpandedText, ShouldFormatMacrosPlist);
+ return FormattedMacroText;
}
+
+static std::optional<StringRef>
+getFormattedMacro(std::optional<StringRef> ExpandedText,
+ bool ShouldFormatMacrosPlist) {
+ if (!ExpandedText) {
+ return std::nullopt;
+ }
+
+ if (!ShouldFormatMacrosPlist) {
+ return *ExpandedText;
+ }
+
+ clang::format::FormatStyle Style = clang::format::getLLVMStyle();
+
+ std::string MacroCodeBlock = ExpandedText->str();
+
+ std::vector<clang::tooling::Range> Ranges;
+ Ranges.emplace_back(0, MacroCodeBlock.length());
+
+ auto Replacements = clang::format::reformat(Style, MacroCodeBlock, Ranges,
+ "<macro-expansion>");
+
+ auto Result =
+ clang::tooling::applyAllReplacements(MacroCodeBlock, Replacements);
+ if (!Result) {
+ return *ExpandedText;
+ }
+
+ static std::vector<std::string> FormattedResults;
+ FormattedResults.emplace_back(*Result);
+
+ return StringRef(FormattedResults.back());
+}
\ No newline at end of file
|
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: None (ankurkraj) ChangesFixes Issue#154743 Full diff: https://github.com/llvm/llvm-project/pull/156046.diff 5 Files Affected:
diff --git a/clang/include/clang/Analysis/PathDiagnostic.h b/clang/include/clang/Analysis/PathDiagnostic.h
index 5907df022e449..665edcf8756ad 100644
--- a/clang/include/clang/Analysis/PathDiagnostic.h
+++ b/clang/include/clang/Analysis/PathDiagnostic.h
@@ -68,6 +68,11 @@ struct PathDiagnosticConsumerOptions {
/// without re-compiling the program under analysis.
bool ShouldDisplayMacroExpansions = false;
+ /// Whether to include clang-formatted macros during macro expansions
+ /// or to keep it as it was stored, the default setting is to kee it as
+ /// is.
+ bool ShouldFormatMacrosPlist = false;
+
/// Whether to include LLVM statistics of the process in the diagnostic.
/// Useful for profiling the tool on large real-world codebases.
bool ShouldSerializeStats = false;
diff --git a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
index 90b80e5201aa8..3f19e3020ba42 100644
--- a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
+++ b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
@@ -351,6 +351,11 @@ ANALYZER_OPTION(bool, ShouldDisplayMacroExpansions, "expand-macros",
"expanded and included in the plist output.",
false)
+ANALYZER_OPTION(bool, ShouldFormatMacrosPlist, "format-macros-plist",
+ "Whether the macros being displayed in the plist "
+ "files are clang-formatted .",
+ false)
+
ANALYZER_OPTION(bool, DisplayCTUProgress, "display-ctu-progress",
"Whether to emit verbose output about "
"the analyzer's progress related to ctu.",
diff --git a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
index 7d0c2d8658f35..d8f32d659a6a1 100644
--- a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
+++ b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
@@ -403,16 +403,14 @@ class AnalyzerOptions {
bool mayInlineCXXMemberFunction(CXXInlineableMemberKind K) const;
ento::PathDiagnosticConsumerOptions getDiagOpts() const {
- return {FullCompilerInvocation,
- ShouldDisplayMacroExpansions,
- ShouldSerializeStats,
+ return {FullCompilerInvocation, ShouldDisplayMacroExpansions,
+ ShouldFormatMacrosPlist, ShouldSerializeStats,
// The stable report filename option is deprecated because
// file names are now always stable. Now the old option acts as
// an alias to the new verbose filename option because this
// closely mimics the behavior under the old option.
ShouldWriteStableReportFilename || ShouldWriteVerboseReportFilename,
- static_cast<bool>(AnalyzerWerror),
- ShouldApplyFixIts,
+ static_cast<bool>(AnalyzerWerror), ShouldApplyFixIts,
ShouldDisplayCheckerNameForText};
}
};
diff --git a/clang/lib/StaticAnalyzer/Core/CMakeLists.txt b/clang/lib/StaticAnalyzer/Core/CMakeLists.txt
index d0a9b202f9c52..6d51fcd0e7cc8 100644
--- a/clang/lib/StaticAnalyzer/Core/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Core/CMakeLists.txt
@@ -60,6 +60,7 @@ add_clang_library(clangStaticAnalyzerCore
clangAnalysis
clangBasic
clangCrossTU
+ clangFormat
clangFrontend
clangLex
clangRewrite
diff --git a/clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp b/clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
index 771d09e19f178..d38e6491c1ce3 100644
--- a/clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
+++ b/clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -17,6 +17,7 @@
#include "clang/Basic/SourceManager.h"
#include "clang/Basic/Version.h"
#include "clang/CrossTU/CrossTranslationUnit.h"
+#include "clang/Format/Format.h"
#include "clang/Frontend/ASTUnit.h"
#include "clang/Lex/Preprocessor.h"
#include "clang/Lex/TokenConcatenation.h"
@@ -100,7 +101,8 @@ class PlistPrinter {
/// is found through a call piece, etc), it's subpieces are reported, and the
/// piece itself is collected. Call this function after the entire bugpath
/// was reported.
- void ReportMacroExpansions(raw_ostream &o, unsigned indent);
+ void ReportMacroExpansions(raw_ostream &o, unsigned indent,
+ bool ShouldFormatMacrosPlist);
private:
void ReportPiece(raw_ostream &o, const PathDiagnosticPiece &P,
@@ -163,9 +165,15 @@ static void printCoverage(const PathDiagnostic *D,
FIDMap &FM,
llvm::raw_fd_ostream &o);
-static std::optional<StringRef> getExpandedMacro(
- SourceLocation MacroLoc, const cross_tu::CrossTranslationUnitContext &CTU,
- const MacroExpansionContext &MacroExpansions, const SourceManager &SM);
+static std::optional<StringRef>
+getExpandedMacro(SourceLocation MacroLoc,
+ const cross_tu::CrossTranslationUnitContext &CTU,
+ const MacroExpansionContext &MacroExpansions,
+ const SourceManager &SM, bool ShouldFormatMacrosPlist);
+
+static std::optional<StringRef>
+getFormattedMacro(std::optional<StringRef> ExpandedText,
+ bool ShouldFormatMacrosPlist);
//===----------------------------------------------------------------------===//
// Methods of PlistPrinter.
@@ -372,7 +380,8 @@ void PlistPrinter::ReportMacroSubPieces(raw_ostream &o,
"Fixits on constrol flow pieces are not implemented yet!");
}
-void PlistPrinter::ReportMacroExpansions(raw_ostream &o, unsigned indent) {
+void PlistPrinter::ReportMacroExpansions(raw_ostream &o, unsigned indent,
+ bool ShouldFormatMacrosPlist) {
for (const PathDiagnosticMacroPiece *P : MacroPieces) {
const SourceManager &SM = PP.getSourceManager();
@@ -382,8 +391,8 @@ void PlistPrinter::ReportMacroExpansions(raw_ostream &o, unsigned indent) {
const std::optional<StringRef> MacroName =
MacroExpansions.getOriginalText(MacroExpansionLoc);
- const std::optional<StringRef> ExpansionText =
- getExpandedMacro(MacroExpansionLoc, CTU, MacroExpansions, SM);
+ const std::optional<StringRef> ExpansionText = getExpandedMacro(
+ MacroExpansionLoc, CTU, MacroExpansions, SM, ShouldFormatMacrosPlist);
if (!MacroName || !ExpansionText)
continue;
@@ -602,7 +611,8 @@ void PlistDiagnostics::printBugPath(llvm::raw_ostream &o, const FIDMap &FM,
o << " <key>macro_expansions</key>\n"
" <array>\n";
- Printer.ReportMacroExpansions(o, /* indent */ 4);
+ Printer.ReportMacroExpansions(o, /* indent */ 4,
+ DiagOpts.ShouldFormatMacrosPlist);
o << " </array>\n";
}
@@ -824,10 +834,49 @@ static std::optional<StringRef>
getExpandedMacro(SourceLocation MacroExpansionLoc,
const cross_tu::CrossTranslationUnitContext &CTU,
const MacroExpansionContext &MacroExpansions,
- const SourceManager &SM) {
+ const SourceManager &SM, bool ShouldFormatMacrosPlist) {
+ std::optional<StringRef> ExpandedText;
if (auto CTUMacroExpCtx =
CTU.getMacroExpansionContextForSourceLocation(MacroExpansionLoc)) {
- return CTUMacroExpCtx->getExpandedText(MacroExpansionLoc);
+ ExpandedText = CTUMacroExpCtx->getExpandedText(MacroExpansionLoc);
+ } else {
+ ExpandedText = MacroExpansions.getExpandedText(MacroExpansionLoc);
}
- return MacroExpansions.getExpandedText(MacroExpansionLoc);
+
+ std::optional<StringRef> FormattedMacroText =
+ getFormattedMacro(ExpandedText, ShouldFormatMacrosPlist);
+ return FormattedMacroText;
}
+
+static std::optional<StringRef>
+getFormattedMacro(std::optional<StringRef> ExpandedText,
+ bool ShouldFormatMacrosPlist) {
+ if (!ExpandedText) {
+ return std::nullopt;
+ }
+
+ if (!ShouldFormatMacrosPlist) {
+ return *ExpandedText;
+ }
+
+ clang::format::FormatStyle Style = clang::format::getLLVMStyle();
+
+ std::string MacroCodeBlock = ExpandedText->str();
+
+ std::vector<clang::tooling::Range> Ranges;
+ Ranges.emplace_back(0, MacroCodeBlock.length());
+
+ auto Replacements = clang::format::reformat(Style, MacroCodeBlock, Ranges,
+ "<macro-expansion>");
+
+ auto Result =
+ clang::tooling::applyAllReplacements(MacroCodeBlock, Replacements);
+ if (!Result) {
+ return *ExpandedText;
+ }
+
+ static std::vector<std::string> FormattedResults;
+ FormattedResults.emplace_back(*Result);
+
+ return StringRef(FormattedResults.back());
+}
\ No newline at end of file
|
|
@llvm/pr-subscribers-clang Author: None (ankurkraj) ChangesFixes Issue#154743 Full diff: https://github.com/llvm/llvm-project/pull/156046.diff 5 Files Affected:
diff --git a/clang/include/clang/Analysis/PathDiagnostic.h b/clang/include/clang/Analysis/PathDiagnostic.h
index 5907df022e449..665edcf8756ad 100644
--- a/clang/include/clang/Analysis/PathDiagnostic.h
+++ b/clang/include/clang/Analysis/PathDiagnostic.h
@@ -68,6 +68,11 @@ struct PathDiagnosticConsumerOptions {
/// without re-compiling the program under analysis.
bool ShouldDisplayMacroExpansions = false;
+ /// Whether to include clang-formatted macros during macro expansions
+ /// or to keep it as it was stored, the default setting is to kee it as
+ /// is.
+ bool ShouldFormatMacrosPlist = false;
+
/// Whether to include LLVM statistics of the process in the diagnostic.
/// Useful for profiling the tool on large real-world codebases.
bool ShouldSerializeStats = false;
diff --git a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
index 90b80e5201aa8..3f19e3020ba42 100644
--- a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
+++ b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
@@ -351,6 +351,11 @@ ANALYZER_OPTION(bool, ShouldDisplayMacroExpansions, "expand-macros",
"expanded and included in the plist output.",
false)
+ANALYZER_OPTION(bool, ShouldFormatMacrosPlist, "format-macros-plist",
+ "Whether the macros being displayed in the plist "
+ "files are clang-formatted .",
+ false)
+
ANALYZER_OPTION(bool, DisplayCTUProgress, "display-ctu-progress",
"Whether to emit verbose output about "
"the analyzer's progress related to ctu.",
diff --git a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
index 7d0c2d8658f35..d8f32d659a6a1 100644
--- a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
+++ b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
@@ -403,16 +403,14 @@ class AnalyzerOptions {
bool mayInlineCXXMemberFunction(CXXInlineableMemberKind K) const;
ento::PathDiagnosticConsumerOptions getDiagOpts() const {
- return {FullCompilerInvocation,
- ShouldDisplayMacroExpansions,
- ShouldSerializeStats,
+ return {FullCompilerInvocation, ShouldDisplayMacroExpansions,
+ ShouldFormatMacrosPlist, ShouldSerializeStats,
// The stable report filename option is deprecated because
// file names are now always stable. Now the old option acts as
// an alias to the new verbose filename option because this
// closely mimics the behavior under the old option.
ShouldWriteStableReportFilename || ShouldWriteVerboseReportFilename,
- static_cast<bool>(AnalyzerWerror),
- ShouldApplyFixIts,
+ static_cast<bool>(AnalyzerWerror), ShouldApplyFixIts,
ShouldDisplayCheckerNameForText};
}
};
diff --git a/clang/lib/StaticAnalyzer/Core/CMakeLists.txt b/clang/lib/StaticAnalyzer/Core/CMakeLists.txt
index d0a9b202f9c52..6d51fcd0e7cc8 100644
--- a/clang/lib/StaticAnalyzer/Core/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Core/CMakeLists.txt
@@ -60,6 +60,7 @@ add_clang_library(clangStaticAnalyzerCore
clangAnalysis
clangBasic
clangCrossTU
+ clangFormat
clangFrontend
clangLex
clangRewrite
diff --git a/clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp b/clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
index 771d09e19f178..d38e6491c1ce3 100644
--- a/clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
+++ b/clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -17,6 +17,7 @@
#include "clang/Basic/SourceManager.h"
#include "clang/Basic/Version.h"
#include "clang/CrossTU/CrossTranslationUnit.h"
+#include "clang/Format/Format.h"
#include "clang/Frontend/ASTUnit.h"
#include "clang/Lex/Preprocessor.h"
#include "clang/Lex/TokenConcatenation.h"
@@ -100,7 +101,8 @@ class PlistPrinter {
/// is found through a call piece, etc), it's subpieces are reported, and the
/// piece itself is collected. Call this function after the entire bugpath
/// was reported.
- void ReportMacroExpansions(raw_ostream &o, unsigned indent);
+ void ReportMacroExpansions(raw_ostream &o, unsigned indent,
+ bool ShouldFormatMacrosPlist);
private:
void ReportPiece(raw_ostream &o, const PathDiagnosticPiece &P,
@@ -163,9 +165,15 @@ static void printCoverage(const PathDiagnostic *D,
FIDMap &FM,
llvm::raw_fd_ostream &o);
-static std::optional<StringRef> getExpandedMacro(
- SourceLocation MacroLoc, const cross_tu::CrossTranslationUnitContext &CTU,
- const MacroExpansionContext &MacroExpansions, const SourceManager &SM);
+static std::optional<StringRef>
+getExpandedMacro(SourceLocation MacroLoc,
+ const cross_tu::CrossTranslationUnitContext &CTU,
+ const MacroExpansionContext &MacroExpansions,
+ const SourceManager &SM, bool ShouldFormatMacrosPlist);
+
+static std::optional<StringRef>
+getFormattedMacro(std::optional<StringRef> ExpandedText,
+ bool ShouldFormatMacrosPlist);
//===----------------------------------------------------------------------===//
// Methods of PlistPrinter.
@@ -372,7 +380,8 @@ void PlistPrinter::ReportMacroSubPieces(raw_ostream &o,
"Fixits on constrol flow pieces are not implemented yet!");
}
-void PlistPrinter::ReportMacroExpansions(raw_ostream &o, unsigned indent) {
+void PlistPrinter::ReportMacroExpansions(raw_ostream &o, unsigned indent,
+ bool ShouldFormatMacrosPlist) {
for (const PathDiagnosticMacroPiece *P : MacroPieces) {
const SourceManager &SM = PP.getSourceManager();
@@ -382,8 +391,8 @@ void PlistPrinter::ReportMacroExpansions(raw_ostream &o, unsigned indent) {
const std::optional<StringRef> MacroName =
MacroExpansions.getOriginalText(MacroExpansionLoc);
- const std::optional<StringRef> ExpansionText =
- getExpandedMacro(MacroExpansionLoc, CTU, MacroExpansions, SM);
+ const std::optional<StringRef> ExpansionText = getExpandedMacro(
+ MacroExpansionLoc, CTU, MacroExpansions, SM, ShouldFormatMacrosPlist);
if (!MacroName || !ExpansionText)
continue;
@@ -602,7 +611,8 @@ void PlistDiagnostics::printBugPath(llvm::raw_ostream &o, const FIDMap &FM,
o << " <key>macro_expansions</key>\n"
" <array>\n";
- Printer.ReportMacroExpansions(o, /* indent */ 4);
+ Printer.ReportMacroExpansions(o, /* indent */ 4,
+ DiagOpts.ShouldFormatMacrosPlist);
o << " </array>\n";
}
@@ -824,10 +834,49 @@ static std::optional<StringRef>
getExpandedMacro(SourceLocation MacroExpansionLoc,
const cross_tu::CrossTranslationUnitContext &CTU,
const MacroExpansionContext &MacroExpansions,
- const SourceManager &SM) {
+ const SourceManager &SM, bool ShouldFormatMacrosPlist) {
+ std::optional<StringRef> ExpandedText;
if (auto CTUMacroExpCtx =
CTU.getMacroExpansionContextForSourceLocation(MacroExpansionLoc)) {
- return CTUMacroExpCtx->getExpandedText(MacroExpansionLoc);
+ ExpandedText = CTUMacroExpCtx->getExpandedText(MacroExpansionLoc);
+ } else {
+ ExpandedText = MacroExpansions.getExpandedText(MacroExpansionLoc);
}
- return MacroExpansions.getExpandedText(MacroExpansionLoc);
+
+ std::optional<StringRef> FormattedMacroText =
+ getFormattedMacro(ExpandedText, ShouldFormatMacrosPlist);
+ return FormattedMacroText;
}
+
+static std::optional<StringRef>
+getFormattedMacro(std::optional<StringRef> ExpandedText,
+ bool ShouldFormatMacrosPlist) {
+ if (!ExpandedText) {
+ return std::nullopt;
+ }
+
+ if (!ShouldFormatMacrosPlist) {
+ return *ExpandedText;
+ }
+
+ clang::format::FormatStyle Style = clang::format::getLLVMStyle();
+
+ std::string MacroCodeBlock = ExpandedText->str();
+
+ std::vector<clang::tooling::Range> Ranges;
+ Ranges.emplace_back(0, MacroCodeBlock.length());
+
+ auto Replacements = clang::format::reformat(Style, MacroCodeBlock, Ranges,
+ "<macro-expansion>");
+
+ auto Result =
+ clang::tooling::applyAllReplacements(MacroCodeBlock, Replacements);
+ if (!Result) {
+ return *ExpandedText;
+ }
+
+ static std::vector<std::string> FormattedResults;
+ FormattedResults.emplace_back(*Result);
+
+ return StringRef(FormattedResults.back());
+}
\ No newline at end of file
|
|
@steakhal Can you please review this PR . |
NagyDonat
left a comment
NagyDonat
left a comment
There was a problem hiding this comment.
This is a very promising change! I can recall some analyzer reports that just dumped lots of macro code without any formatting.
Could you add a few testcases that would demonstrate the effects of your change? (The tests of the analyzer are in clang/test/Analysis, IIRC there are some testcases that test plist output, you can follow their approach.)
steakhal
left a comment
steakhal
left a comment
There was a problem hiding this comment.
Looks great. This should be tested in the unittests, somehwere the sibling APIs are tested.
|
Speaking of testing, we should probably not overdo it. Clang format is a different component. They likely want to remain flexible. If we pin their output too hard, they would become unhappy long term. Speaking of the plist test, I frankly hate them. If we can get away with just unittest, I'd prefer those instead. |
|
Can't find any unit tests for methods in PlistDiagnostics.cpp So adding the unit test in |
Yes, I had Wrt. the second part; would it make sense to move this formatting functionality into the |
Sorry for the delay. I have made the following changes @steakhal Can you please review it and let me know if any other changes are required ? |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
NagyDonat
left a comment
NagyDonat
left a comment
There was a problem hiding this comment.
Thanks for the updates!
The PR looks good to me overall and I am very happy to see the progress; I just marked a very minor suggestions in inline comments.
| ANALYZER_OPTION(bool, ShouldFormatMacrosPlist, "format-macros-plist", | ||
| "Whether the macros being displayed in the plist " | ||
| "files are clang-formatted .", | ||
| false) |
There was a problem hiding this comment.
This seems to be a helpful and harmless option, so I don't see a reason why we shouldn't enable it by default. @steakhal @ anybody else WDYT about this? Can you imagine anything that would be broken by the pretty-printing of macros?
There was a problem hiding this comment.
I don't see why plist are treated differently. Through that lens, I don't see the justification for this option.
IMO we should just always return the formatted macros for all diagnostic consumers - including but not limited to the plist generator.
Maybe provide an escape hatch in the unforeseen case that clang-format would crash on something.
There was a problem hiding this comment.
I tried removing the options and making this the default setting, some plist tests are failing thuogh.
I gather these expect the macro expansions to be present in the same format as the input one.
I cna fix these tests to match the formatted output, but in that case there will be a dependency on clang-format and the tests will need an update if the clang-format has some changes.
How do you guys think I should proceed ?
llvm-project/build/bin/FileCheck --input-file=llvm-project/build/tools/clang/test/Analysis/Output/plist-macros-with-expansion.cpp.tmp.plist llvm-project/clang/test/Analysis/plist-macros-with-expansion.cpp
# executed command: llvm-project/build/bin/FileCheck --input-file=llvm-project/build/tools/clang/test/Analysis/Output/plist-macros-with-expansion.cpp.tmp.plist llvm-project/clang/test/Analysis/plist-macros-with-expansion.cpp
# .---command stderr------------
# | llvm-project/clang/test/Analysis/plist-macros-with-expansion.cpp:32:16: error: CHECK-NEXT: expected string not found in input
# | // CHECK-NEXT: <key>expansion</key><string>ptr =0</string>
# | ^
# | llvm-project/build/tools/clang/test/Analysis/Output/plist-macros-with-expansion.cpp.tmp.plist:148:53: note: scanning from here
# | <key>name</key><string>SET_PTR_VAR_TO_NULL</string>
# | ^
# | llvm-project/build/tools/clang/test/Analysis/Output/plist-macros-with-expansion.cpp.tmp.plist:149:2: note: possible intended match here
# | <key>expansion</key><string>ptr = 0</string>
# | ^
# |
# | Input file: llvm-project/build/tools/clang/test/Analysis/Output/plist-macros-with-expansion.cpp.tmp.plist
# | Check file: llvm-project/clang/test/Analysis/plist-macros-with-expansion.cpp
# |
# | -dump-input=help explains the following input dump.
# |
# | Input was:
# | <<<<<<
# | .
# | .
# | .
# | 143: <dict>
# | 144: <key>line</key><integer>18</integer>
# | 145: <key>col</key><integer>3</integer>
# | 146: <key>file</key><integer>0</integer>
# | 147: </dict>
# | 148: <key>name</key><string>SET_PTR_VAR_TO_NULL</string>
# | next:32'0 X error: no match found
# | 149: <key>expansion</key><string>ptr = 0</string>
# | next:32'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | next:32'1 ? possible intended match
# | 150: </dict>
# | next:32'0 ~~~~~~~~~
# | 151: </array>
# | next:32'0 ~~~~~~~~~~
# | 152: <key>description</key><string>Dereference of null pointer (loaded from variable 'ptr')</string>
# | next:32'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | 153: <key>category</key><string>Logic error</string>
# | next:32'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | 154: <key>type</key><string>Dereference of null pointer</string>
# | next:32'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | .
# | .
# | .
# | >>>>>>
# `-----------------------------
# error: command failed with exit status: 1
--
********************
Testing: 0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
********************
Failed Tests (4):
Clang :: Analysis/analyzer-config.c
Clang :: Analysis/plist-macros-with-expansion-ctu.c
Clang :: Analysis/plist-macros-with-expansion.c
Clang :: Analysis/plist-macros-with-expansion.cpp
There was a problem hiding this comment.
I see your problem. I think there is a way.
How about we only check the prefix of the matched lines. (this is what FileCheck does by default for CHECK lines)
So, I think it's fair to assume that a macro in these tests will start without any leading whitespaces, and then the first token the macro would expand to; and after that just not check the rest of the line.
This way we still have something, and that is still stable to the reasonable degree.
WDYT?
There was a problem hiding this comment.
That seems like a good idea but I have a doubt,
in files like clang/test/Analysis/plist-macros-with-expansion.cpp , there are ~ 80 macro expansions check , there are checks for different scenarios. Will the above check suffice the intentions of the tests, in all such cases ?
( for example there are expansion checkers for macroArgContainsCommaLParenRParenTest, macroArgContainsCommaInStringTest,CALL_FUNCTION_WITH_TWO_PARAMS, CALL_FUNCTION_WITH_ONE_PARAM_THROUGH_MACRO
There was a problem hiding this comment.
Good question. Let's be pragmatic.
What are the chances of having formatting changes of the expansions we are talking about?
If the uplifted test would make sense, I think we should just accept them like that.
If there is any controversial, we can discuss that and decide how to move forward.
I'm open for all kinds of solutions, including removing some tests. IMO the formatting for expansions would be great, and a long awaited feature that worths serious consideration.
There was a problem hiding this comment.
Note that the we have so many test because in the past we used to have some really complicated and buggy logic re-doing parsing and macro substitution. This needed elaborate testing, hence these files.
However, about 5 years ago I replaced that system with what we currently have, which is mature and stable.
So through that lens, many of these tests are not pulling their weight - so we could just drop 95% of them without any consequences if needed.
There was a problem hiding this comment.
Thanks for the explanation. I have removed the option, so formatting is default
I just wanted to understand the intention of so many tests, I have replaced the tests with prefix match wherever applicable ( alot of these do seem redundant though, after the changes ) . Let me know if I need to remove / update any tests here. ( or I could raise a separate PR for this if you want )
There was a problem hiding this comment.
Looks good to me, I think the commit is ready to be merged.
The only remaining question is whether we want to enable this feature by default -- I think it may be worth to wait a bit for other opinion(s?) about that question, but @ankurkraj I'm happy to help you with merging your commit whenever you request it.
EDIT: I see that the code formatting check bot complains about the formatting of your commits (see the autogenerated comment), please fix those stylistic issues. (By the way that bot runs git-clang-format under the hood; you can run git-clang-format $PARENT_OF_YOUR_EARLIEST_COMMIT yourself to automatically reformat your changes to the style enforced by the bot. Note that git-clang-format is safe to use this way, it won't run if your repo has changes that are not yet stored in git.)
steakhal
left a comment
steakhal
left a comment
There was a problem hiding this comment.
This looks really good. I like it.
Seem my inline comments.
| ANALYZER_OPTION(bool, ShouldFormatMacrosPlist, "format-macros-plist", | ||
| "Whether the macros being displayed in the plist " | ||
| "files are clang-formatted .", | ||
| false) |
There was a problem hiding this comment.
I don't see why plist are treated differently. Through that lens, I don't see the justification for this option.
IMO we should just always return the formatted macros for all diagnostic consumers - including but not limited to the plist generator.
Maybe provide an escape hatch in the unforeseen case that clang-format would crash on something.
|
I think we should discuss an earlier comment of mine:
Fundamentally, the question is why should we have two separate APIs for getting the expanded text? |
There is already a fallback mechanism in the formatting part (in case the clang-format fails on the expansion ) , so IMO it shouldn't be a problem to replace the original API with the new one. Let me know if you guys think I should get rid of it |
Yes, I think so. WDYT @NagyDonat ? |
|
I also support unconditionally returning formatted macros for all consumers. My only potential concern is that there could be some outlier macros where the pretty-printed layout is (visually) inflated (e.g. perhaps something like |
| clangAST | ||
| clangASTMatchers | ||
| clangBasic | ||
| clangFormat |
There was a problem hiding this comment.
Overall, I am supportive of using formatted text in reports. But I think we need an RFC in discourse before we link format with the main Clang library.
There was a problem hiding this comment.
5 participants
Fixes Issue#154743