[PATCH] D121446: Add -no-unused-coverage option

[Hiral via Phabricator via cfe-commits]

Hiralo created this revision.
Hiralo added a reviewer: vsk.
Hiralo added a project: clang.
Herald added subscribers: dexonsmith, wenlei, dang.
Herald added a project: All.
Hiralo requested review of this revision.
Herald added a subscriber: cfe-commits.

commit 87f30be546daca55664ded147acceb9f206399e0
Author: Hiral Oza 
Date:   Fri Mar 11 09:41:38 2022 +0530

  Add -no-unused-coverage option
  
  Reduce the size of instrumented objects by eliminating counters for code that 
will not be emitted.
  
  Author: thom.h...@netapp.com

diff --git a/clang/include/clang/Basic/CodeGenOptions.def 
b/clang/include/clang/Basic/CodeGenOptions.def
index 3301e7587d21..e51261ddcced 100644

- a/clang/include/clang/Basic/CodeGenOptions.def

+++ b/clang/include/clang/Basic/CodeGenOptions.def
@@ -211,6 +211,8 @@ CODEGENOPT(CoverageMapping , 1, 0) ///< Generate coverage 
mapping regions to

  ///< enable code coverage analysis.

CODEGENOPT(DumpCoverageMapping , 1, 0) ///< Dump the generated coverage mapping

  ///< regions.

+CODEGENOPT(NoUnusedCoverage , 1, 0) ///< Turn off coverage mapping for code 
that
+///< is not emitted.

  /// If -fpcc-struct-return or -freg-struct-return is specified.

ENUM_CODEGENOPT(StructReturnConvention, StructReturnConventionKind, 2, 
SRCK_Default)
diff --git a/clang/include/clang/Driver/Options.td 
b/clang/include/clang/Driver/Options.td
index fa54d4a7689c..05926e01efb1 100644

- a/clang/include/clang/Driver/Options.td

+++ b/clang/include/clang/Driver/Options.td
@@ -5158,6 +5158,9 @@ def coverage_version_EQ : Joined<["-"], 
"coverage-version=">,
 def dump_coverage_mapping : Flag<["-"], "dump-coverage-mapping">,

  HelpText<"Dump the coverage mapping records, for testing">,
  MarshallingInfoFlag>;

+def no_unused_coverage : Flag<["-"], "no-unused-coverage">,
+  HelpText<"Turn off coverage mapping for code that is not emitted">,
+  MarshallingInfoFlag>;
 def fuse_register_sized_bitfield_access: Flag<["-"], 
"fuse-register-sized-bitfield-access">,

  HelpText<"Use register sized accesses to bit-fields, when possible.">,
  MarshallingInfoFlag>;

diff --git a/clang/lib/CodeGen/CodeGenModule.cpp 
b/clang/lib/CodeGen/CodeGenModule.cpp
index 9c76648f5f19..bc968d888e11 100644

- a/clang/lib/CodeGen/CodeGenModule.cpp

+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -538,7 +538,9 @@ void CodeGenModule::Release() {

  EmitCtorList(GlobalDtors, "llvm.global_dtors");
  EmitGlobalAnnotations();
  EmitStaticExternCAliases();

- EmitDeferredUnusedCoverageMappings();

+  if (!CodeGenOpts.NoUnusedCoverage) {
+EmitDeferredUnusedCoverageMappings();
+  }

  CodeGenPGO(*this).setValueProfilingFlag(getModule());
  if (CoverageMapping)
CoverageMapping->emit();


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D121446

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CodeGenModule.cpp


Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -538,7 +538,9 @@
   EmitCtorList(GlobalDtors, "llvm.global_dtors");
   EmitGlobalAnnotations();
   EmitStaticExternCAliases();
-  EmitDeferredUnusedCoverageMappings();
+  if (!CodeGenOpts.NoUnusedCoverage) {
+EmitDeferredUnusedCoverageMappings();
+  }
   CodeGenPGO(*this).setValueProfilingFlag(getModule());
   if (CoverageMapping)
 CoverageMapping->emit();
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -5158,6 +5158,9 @@
 def dump_coverage_mapping : Flag<["-"], "dump-coverage-mapping">,
   HelpText<"Dump the coverage mapping records, for testing">,
   MarshallingInfoFlag>;
+def no_unused_coverage : Flag<["-"], "no-unused-coverage">,
+  HelpText<"Turn off coverage mapping for code that is not emitted">,
+  MarshallingInfoFlag>;
 def fuse_register_sized_bitfield_access: Flag<["-"], 
"fuse-register-sized-bitfield-access">,
   HelpText<"Use register sized accesses to bit-fields, when possible.">,
   MarshallingInfoFlag>;
Index: clang/include/clang/Basic/CodeGenOptions.def
===
--- clang/include/clang/Basic/CodeGenOptions.def
+++ clang/include/clang/Basic/CodeGenOptions.def
@@ -211,6 +211,8 @@
///< enable code coverage analysis.
 CODEGENOPT(DumpCoverageMapping , 1, 0) ///< Dump the generated coverage mapping
///< regions.
+CODEGENOPT(NoUnusedCoverage , 1, 0) ///< Turn off coverage mapping for code 
that
+///< is not emitted.

   /// If -fpcc-struct-return or -freg-struct-return is specified.
 ENUM_CODEGENOPT(StructReturnConvention, StructReturnConventionKind, 2, 
SRCK_Default)


Index: clang/lib/CodeGen/

[PATCH] D121457: Clamp negative coverage counters to zero.

[Hiral via Phabricator via cfe-commits]

Hiralo created this revision.
Hiralo added a reviewer: vsk.
Hiralo added projects: clang, LLVM.
Herald added subscribers: dexonsmith, wenlei, dang, kbarton, nemanjai.
Herald added a project: All.
Hiralo requested review of this revision.
Herald added subscribers: llvm-commits, cfe-commits.

commit 60009ab7a6491095b99acd1445dfe0fc5418cb3f
Author: Hiral Oza 
Date:   Fri Mar 11 10:19:51 2022 +0530

  Clamp negative coverage counters to zero.
  
  Because there is no locking on counter updates, race conditions may
  cause counters to become negative. Clamp them to zero when exporting
  LCOV reports.
  
  Optionally remove blank lines from coverage
  
  Added -omit-blank-lines option to add Skip regions to the coverage map
  for lines of source code that are blank.
  
  Author: thom.h...@netapp.com


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D121457

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Lex/PPCallbacks.h
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/CodeGen/CodeGenPGO.cpp
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/lib/Lex/Preprocessor.cpp
  llvm/tools/llvm-cov/CoverageExporterLcov.cpp

Index: llvm/tools/llvm-cov/CoverageExporterLcov.cpp
===
--- llvm/tools/llvm-cov/CoverageExporterLcov.cpp
+++ llvm/tools/llvm-cov/CoverageExporterLcov.cpp
@@ -70,7 +70,10 @@
   for (; LCI != LCIEnd; ++LCI) {
 const coverage::LineCoverageStats &LCS = *LCI;
 if (LCS.isMapped()) {
-  OS << "DA:" << LCS.getLine() << ',' << LCS.getExecutionCount() << '\n';
+  OS << "DA:" << LCS.getLine() << ','
+ << ((LCS.getExecutionCount() & (1ULL << 63)) ? 0
+  : LCS.getExecutionCount())
+ << '\n';
 }
   }
 }
Index: clang/lib/Lex/Preprocessor.cpp
===
--- clang/lib/Lex/Preprocessor.cpp
+++ clang/lib/Lex/Preprocessor.cpp
@@ -964,6 +964,31 @@
 }
   }

+  if (Callbacks) {
+SourceLocation TokenLoc = Result.getLocation();
+if (TokenLoc.isFileID() &&
+!SourceMgr.isInSystemHeader(SourceMgr.getSpellingLoc(TokenLoc))) {
+  bool Invalid = false;
+  unsigned LineNumber = SourceMgr.getSpellingLineNumber(TokenLoc, &Invalid);
+  if (!Invalid) {
+FileID FID = SourceMgr.getFileID(TokenLoc);
+HighestLineNumberMap::iterator fi = HighestLineNumbers.find(FID);
+unsigned HighestLineNumber =
+fi == HighestLineNumbers.end() ? 0 : fi->second;
+if (LineNumber > HighestLineNumber + 1) {
+  SourceLocation BlankStart =
+  SourceMgr.translateLineCol(FID, HighestLineNumber + 1, 1);
+  SourceLocation BlankEnd =
+  SourceMgr.translateLineCol(FID, LineNumber - 1, 1);
+  Callbacks->BlankLines(SourceRange(BlankStart, BlankEnd));
+}
+if (LineNumber > HighestLineNumber) {
+  HighestLineNumbers[FID] = LineNumber;
+}
+  }
+}
+  }
+
   LastTokenWasAt = Result.is(tok::at);
   --LexLevel;

Index: clang/lib/CodeGen/CoverageMappingGen.h
===
--- clang/lib/CodeGen/CoverageMappingGen.h
+++ clang/lib/CodeGen/CoverageMappingGen.h
@@ -23,6 +23,7 @@

 namespace clang {

+class CodeGenOptions;
 class LangOptions;
 class SourceManager;
 class FileEntry;
@@ -50,6 +51,7 @@
public EmptylineHandler {
   // A vector of skipped source ranges and PrevTokLoc with NextTokLoc.
   std::vector SkippedRanges;
+  std::vector BlankLineRanges;

   SourceManager &SourceMgr;

@@ -61,8 +63,10 @@
   CoverageSourceInfo(SourceManager &SourceMgr) : SourceMgr(SourceMgr) {}

   std::vector &getSkippedRanges() { return SkippedRanges; }
+  ArrayRef getBlankLines() const { return BlankLineRanges; }

   void AddSkippedRange(SourceRange Range);
+  void BlankLines(SourceRange Range) override;

   void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) override;

@@ -135,18 +139,23 @@
 class CoverageMappingGen {
   CoverageMappingModuleGen &CVM;
   SourceManager &SM;
+  const CodeGenOptions &CodeGenOpts;
   const LangOptions &LangOpts;
   llvm::DenseMap *CounterMap;

 public:
   CoverageMappingGen(CoverageMappingModuleGen &CVM, SourceManager &SM,
+ const CodeGenOptions &CodeGenOpts,
  const LangOptions &LangOpts)
-  : CVM(CVM), SM(SM), LangOpts(LangOpts), CounterMap(nullptr) {}
+  : CVM(CVM), SM(SM), CodeGenOpts(CodeGenOpts), LangOpts(LangOpts),
+CounterMap(nullptr) {}

   CoverageMappingGen(CoverageMappingModuleGen &CVM, SourceManager &SM,
+ const CodeGenOptions &CodeGenOpts,
  const LangOptions &LangOpts,
  llvm::DenseMap *CounterMap)
-  : CVM(CVM), SM(SM), LangOpts(LangOpts), CounterM

[PATCH] D121457: Clamp negative coverage counters to zero.

[Hiral via Phabricator via cfe-commits]

Hiralo added a comment.

In D121457#3375039 , @lebedev.ri 
wrote:

> Wouldn't it be best to instead fix the non-thread-safety of the coverage 
> counters?

Can you please elaborate or point me to code? Thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121457

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

[PATCH] D34654: Allow passing a regex for headers to exclude from clang-tidy

[Hiral via Phabricator via cfe-commits]

Hiralo added a comment.

We are dependent on this patch to exclude headers in regex while running 
clang-tidy.
Can we have this patch merged with upcoming GA versions? or
Can someone summarize what is pending in this review or what could be good 
alternative in v11 clang-tidy?
Thank you in advance.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D34654

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

[PATCH] D89936: [clang-tidy] adding "--config-file=" to specify custom config file.

[Hiral via Phabricator via cfe-commits]

Hiralo added a comment.

Hello @DmitryPolukhin ,
Sorry I missed to update 'SUMMARY' section of this review earlier! was not 
aware that that will be considered as commit message.
I have now updated the 'SUMMARY', it is possible to revert this commit and then 
re-submit it with updated commit message?
I think latest commit message reflects and provides details about usage and 
helpful to people.
Sorry for inconvenience caused!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89936

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

[PATCH] D89936: [clang-tidy] adding "--config-file=" to specify custom config file.

[Hiral via Phabricator via cfe-commits]

Hiralo added a comment.

Got it. No worries :)
Thanks @DmitryPolukhin for your help.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89936

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

[PATCH] D89936: clang-tidy: adding "--clang-tidy-config=" to specify custom config file

[Hiral via Phabricator via cfe-commits]

Hiralo created this revision.
Hiralo added reviewers: alexfh, njames93, hokein, DmitryPolukhin.
Hiralo added a project: clang-tools-extra.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Hiralo requested review of this revision.

commit 5c98467825b128aabd04a770ac26905d856a3757
Author: Hiral Oza 
Date:   Wed Oct 21 23:30:22 2020 -0700

  clang-tidy: adding "--clang-tidy-config=" to specify custom config 
file.
  
  Let clang-tidy to read config from specified file.
  Example:
   $ clang-tidy --clang-tidy-config=/some/path/myTidyConfig --list-checks --
 ...this will read config from '/some/path/myTidyConfig'.
  
  May speed-up tidy runtime since now it will just look-up 
  instead of searching ".clang-tidy" in parent-dir(s).
  
  Directly specifying config path helps setting build dependencies.

Thank you in advance for your kind review.
-Hiral


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89936

Files:
  clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
  clang-tools-extra/clang-tidy/ClangTidyOptions.h
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp

Index: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -73,6 +73,13 @@
 )"),
cl::init(""), cl::cat(ClangTidyCategory));
 
+static cl::opt ClangTidyConfig("clang-tidy-config", cl::desc(R"(
+Specify full path of .clang-tidy config file.
+For example, --clang-tidy-config=/some/path/myTidyConfig
+)"),
+cl::init(""),
+cl::cat(ClangTidyCategory));
+
 static cl::opt WarningsAsErrors("warnings-as-errors", cl::desc(R"(
 Upgrades warnings to errors. Same format as
 '-checks'.
@@ -279,6 +286,7 @@
 
   ClangTidyOptions DefaultOptions;
   DefaultOptions.Checks = DefaultChecks;
+  DefaultOptions.ClangTidyConfig = "";
   DefaultOptions.WarningsAsErrors = "";
   DefaultOptions.HeaderFilterRegex = HeaderFilter;
   DefaultOptions.SystemHeaders = SystemHeaders;
@@ -291,6 +299,8 @@
   ClangTidyOptions OverrideOptions;
   if (Checks.getNumOccurrences() > 0)
 OverrideOptions.Checks = Checks;
+  if (ClangTidyConfig.getNumOccurrences() > 0)
+OverrideOptions.ClangTidyConfig = ClangTidyConfig;
   if (WarningsAsErrors.getNumOccurrences() > 0)
 OverrideOptions.WarningsAsErrors = WarningsAsErrors;
   if (HeaderFilter.getNumOccurrences() > 0)
Index: clang-tools-extra/clang-tidy/ClangTidyOptions.h
===
--- clang-tools-extra/clang-tidy/ClangTidyOptions.h
+++ clang-tools-extra/clang-tidy/ClangTidyOptions.h
@@ -64,6 +64,9 @@
   /// Checks filter.
   llvm::Optional Checks;
 
+  /// Clang-tidy-config
+  llvm::Optional ClangTidyConfig;
+
   /// WarningsAsErrors filter.
   llvm::Optional WarningsAsErrors;
 
@@ -227,6 +230,8 @@
   /// Try to read configuration files from \p Directory using registered
   /// \c ConfigHandlers.
   llvm::Optional tryReadConfigFile(llvm::StringRef Directory);
+  llvm::Optional tryReadConfigFile(llvm::StringRef Path,
+  bool IsFile);
 
   llvm::StringMap CachedOptions;
   ClangTidyOptions OverrideOptions;
Index: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
@@ -109,6 +109,7 @@
 ClangTidyOptions ClangTidyOptions::getDefaults() {
   ClangTidyOptions Options;
   Options.Checks = "";
+  Options.ClangTidyConfig = "";
   Options.WarningsAsErrors = "";
   Options.HeaderFilterRegex = "";
   Options.SystemHeaders = false;
@@ -306,7 +307,41 @@
   << "...\n");
   assert(FS && "FS must be set.");
 
-  llvm::SmallString<128> AbsoluteFilePath(FileName);
+  llvm::SmallString<1024> AbsoluteFilePath(FileName);
+  if (!OverrideOptions.ClangTidyConfig.getValue().empty()) {
+AbsoluteFilePath.assign(OverrideOptions.ClangTidyConfig.getValue());
+if (FS->makeAbsolute(AbsoluteFilePath)) {
+  std::string Msg;
+  llvm::raw_string_ostream ErrStream(Msg);
+  ErrStream << " reading configuration from <" << AbsoluteFilePath
+<< "> can't make absolute path.\n";
+  llvm::report_fatal_error(ErrStream.str());
+}
+bool IsFile = false;
+bool IsLink = false;
+llvm::sys::fs::is_regular_file(Twine(AbsoluteFilePath), IsFile);
+llvm::sys::fs::is_symlink_file(Twine(AbsoluteFilePath), IsLink);
+if (!(IsFile || IsLink)) {
+  std::string Msg;
+  llvm::raw_string_ostream ErrStream(Msg);
+  ErrStream << " reading configuration from <" << AbsoluteFilePath
+<< "> file doesn't exist or not regular/symlink file.\n";
+  llvm::report_fatal_error(ErrS

[PATCH] D90010: clang-tidy: Reduce number of stderr write calls

[Hiral via Phabricator via cfe-commits]

Hiralo created this revision.
Hiralo added reviewers: alexfh, njames93, hokein, DmitryPolukhin, djasper, 
ilya-biryukov.
Hiralo added a project: clang-tools-extra.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Hiralo requested review of this revision.

commit c3fa7da502d6cc77e5c0710b0e9308e6fbdd137c
Author: Hiral Oza 
Date:   Thu Oct 22 22:06:25 2020 -0700

  clang-tidy: Reduce number of stderr write calls
  
  Making single write call in place of 7 small string write calls.

Thank you in advance for your kind review.
-Hiral


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90010

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


Index: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -241,31 +241,36 @@
 namespace tidy {
 
 static void printStats(const ClangTidyStats &Stats) {
+  std::string Msg;
+  llvm::raw_string_ostream ErrStream(Msg);
+
   if (Stats.errorsIgnored()) {
-llvm::errs() << "Suppressed " << Stats.errorsIgnored() << " warnings (";
+ErrStream << "Suppressed " << Stats.errorsIgnored() << " warnings (";
+
 StringRef Separator = "";
 if (Stats.ErrorsIgnoredNonUserCode) {
-  llvm::errs() << Stats.ErrorsIgnoredNonUserCode << " in non-user code";
+  ErrStream << Stats.ErrorsIgnoredNonUserCode << " in non-user code";
   Separator = ", ";
 }
 if (Stats.ErrorsIgnoredLineFilter) {
-  llvm::errs() << Separator << Stats.ErrorsIgnoredLineFilter
-   << " due to line filter";
+  ErrStream << Separator << Stats.ErrorsIgnoredLineFilter
+<< " due to line filter";
   Separator = ", ";
 }
 if (Stats.ErrorsIgnoredNOLINT) {
-  llvm::errs() << Separator << Stats.ErrorsIgnoredNOLINT << " NOLINT";
+  ErrStream << Separator << Stats.ErrorsIgnoredNOLINT << " NOLINT";
   Separator = ", ";
 }
 if (Stats.ErrorsIgnoredCheckFilter)
-  llvm::errs() << Separator << Stats.ErrorsIgnoredCheckFilter
-   << " with check filters";
-llvm::errs() << ").\n";
+  ErrStream << Separator << Stats.ErrorsIgnoredCheckFilter
+<< " with check filters";
+ErrStream << ").\n";
 if (Stats.ErrorsIgnoredNonUserCode)
-  llvm::errs() << "Use -header-filter=.* to display errors from all "
-  "non-system headers. Use -system-headers to display "
-  "errors from system headers as well.\n";
+  ErrStream << "Use -header-filter=.* to display errors from all "
+   "non-system headers. Use -system-headers to display "
+   "errors from system headers as well.\n";
   }
+  llvm::errs() << ErrStream.str();
 }
 
 static std::unique_ptr createOptionsProvider(


Index: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -241,31 +241,36 @@
 namespace tidy {
 
 static void printStats(const ClangTidyStats &Stats) {
+  std::string Msg;
+  llvm::raw_string_ostream ErrStream(Msg);
+
   if (Stats.errorsIgnored()) {
-llvm::errs() << "Suppressed " << Stats.errorsIgnored() << " warnings (";
+ErrStream << "Suppressed " << Stats.errorsIgnored() << " warnings (";
+
 StringRef Separator = "";
 if (Stats.ErrorsIgnoredNonUserCode) {
-  llvm::errs() << Stats.ErrorsIgnoredNonUserCode << " in non-user code";
+  ErrStream << Stats.ErrorsIgnoredNonUserCode << " in non-user code";
   Separator = ", ";
 }
 if (Stats.ErrorsIgnoredLineFilter) {
-  llvm::errs() << Separator << Stats.ErrorsIgnoredLineFilter
-   << " due to line filter";
+  ErrStream << Separator << Stats.ErrorsIgnoredLineFilter
+<< " due to line filter";
   Separator = ", ";
 }
 if (Stats.ErrorsIgnoredNOLINT) {
-  llvm::errs() << Separator << Stats.ErrorsIgnoredNOLINT << " NOLINT";
+  ErrStream << Separator << Stats.ErrorsIgnoredNOLINT << " NOLINT";
   Separator = ", ";
 }
 if (Stats.ErrorsIgnoredCheckFilter)
-  llvm::errs() << Separator << Stats.ErrorsIgnoredCheckFilter
-   << " with check filters";
-llvm::errs() << ").\n";
+  ErrStream << Separator << Stats.ErrorsIgnoredCheckFilter
+<< " with check filters";
+ErrStream << ").\n";
 if (Stats.ErrorsIgnoredNonUserCode)
-  llvm::errs() << "Use -header-filter=.* to display errors from all "
-  "non-system headers. Use -system-headers to display "
-  "errors from system headers as well.\n";
+  ErrStream << "Use -header-filter=.* to display errors from all "
+   "non-system headers. Use -system-headers to disp

[PATCH] D90010: clang-tidy: Reduce number of stderr write calls

[Hiral via Phabricator via cfe-commits]

Hiralo added a comment.

In D90010#2352556 , @njames93 wrote:

> Isn't `llvm::errs()` buffered, negating most of the benefit here.

If it is buffered, we would expect single write call to stderr. But we are 
seeing 7 write calls!

Yes, please suggest how to make llvm::errs() buffered that will be easy patch :)

In D90010#2352627 , @dblaikie wrote:

> (the patch description doesn't explain any specific motivation either - 
> whether it's performance (runtime? memory usage? etc?) or something else, and 
> how the performance aspect has been quantified)

The motivation is to avoid 7 write calls which helps in large build system and 
easy on NFS!

Thank you.
-Hiral


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90010

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

[PATCH] D89936: clang-tidy: adding "--clang-tidy-config=" to specify custom config file

[Hiral via Phabricator via cfe-commits]

Hiralo added a comment.

Thanks for your kind review.
Working on it...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89936

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

[PATCH] D90010: clang-tidy: Reduce number of stderr write calls

[Hiral via Phabricator via cfe-commits]

Hiralo added a comment.

In D90010#2355432 , @dblaikie wrote:

> Looks like you might be able to do something like 
> "llvm::errs().setBuffered()" ?

Do we need to set it for each function using llvm:errs() (e.g. here 
printStats() )
OR can it be set once for entire clang-tidy ?

>> In D90010#2352627 , @dblaikie wrote:
>>
>>> (the patch description doesn't explain any specific motivation either - 
>>> whether it's performance (runtime? memory usage? etc?) or something else, 
>>> and how the performance aspect has been quantified)
>>
>> The motivation is to avoid 7 write calls which helps in large build system 
>> and easy on NFS!
>
> Sorry, I meant "why does any of this matter" I take it you mean "because 7 
> write calls are slower than 1 in  by  time/percentage>" - do you have rough data/description of the situation where 
> the speed of printing error messages matters and by how much does it matter? 
> (I think it would be good to have this data no matter the solution - be it 
> explicit or built-in buffering)

No worries. Thanks for your valuable reviews.

For example, consider clang-tidy running on 10,000 files... we expect to have 
minimal number of write calls. With the code-as-is makes 10,000 * 7 = 70,000 
stderr write calls with small small chunk of strings!!!
With proposed changes OR with llvm::errs().setBuffered() set we will see only 
10,000 legitimate stderr write calls :)

Thank you.
-Hiral


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90010

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

[PATCH] D90010: clang-tidy: Reduce number of stderr write calls

[Hiral via Phabricator via cfe-commits]

Hiralo added a comment.

Tried using llvm::errs().SetBuffered() within printStats()...


static void printStats(const ClangTidyStats &Stats) {
+  llvm::errs().SetBuffered()


but still I see below stderr write calls...
...
write(2, "10712", 5)= 5
write(2, " warning", 8) = 8
write(2, "s", 1)= 1
write(2, " generated", 10)  = 10
write(2, ".\n", 2)  = 2
...
write(2, "Suppressed ", 11) = 11
write(2, "10703", 5)= 5
write(2, " warnings (", 11) = 11
write(2, "10703", 5)= 5
write(2, " in non-user code", 17)   = 17
write(2, ").\n", 3) = 3
write(2, "Use -header-filter=.* to display"..., 136) = 136
...

We expect it to emit one write call only!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90010

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

[PATCH] D90010: clang-tidy: Reduce number of stderr write calls

[Hiral via Phabricator via cfe-commits]

Hiralo added a comment.

In D90010#2355460 , @dblaikie wrote:

> In D90010#2355443 , @Hiralo wrote:
>
>> In D90010#2355432 , @dblaikie wrote:
>>
>>> Looks like you might be able to do something like 
>>> "llvm::errs().setBuffered()" ?
>>
>> Do we need to set it for each function using llvm:errs() (e.g. here 
>> printStats() )
>> OR can it be set once for entire clang-tidy ?
>
> I think it could be set for the whole process. (though there's a reason 
> stderr isn't usually buffered - because if it's buffered and the process 
> crashes, then you don't get all the output/maybe something important will be 
> lost)

Sure

> Might be worth debugging through it and seeing what's happening, perhaps? I'm 
> not sure exactly why that might not be buffering - perhaps it uses  
> raw_fd_ostream::preferred_buffer_size which might be returning 0 if it's a 
> terminal that it's outputting to? In that case you could try "SetBufferSize" 
> that'll set a customized buffer size and, I think, enable buffering.

I tried setting...
static void printStats(const ClangTidyStats &Stats) {
+  llvm::errs().SetBufferSize(4096);
+  llvm::errs().SetBuffered();

but still didn't work!

>> For example, consider clang-tidy running on 10,000 files... we expect to 
>> have minimal number of write calls. With the code-as-is makes 10,000 * 7 = 
>> 70,000 stderr write calls with small small chunk of strings!!!
>> With proposed changes OR with llvm::errs().setBuffered() set we will see 
>> only 10,000 legitimate stderr write calls :)
>
> Right, but what I mean is "what observable difference do you see" - as a user 
> you aren't counting write calls, you're looking at the wall time, for 
> instance. What difference in user-observable behavior are you seeing/are you 
> trying to address, and how much does this change help that user-observable 
> behavior?
>
> I don't know too much about clang-tidy, but the statistics output for 10,000 
> files doesn't seem like it'd be that useful, would it? So maybe the solution 
> is to turn it off, rather than to make it faster? (generally error paths are 
> thought of as not needing to be efficient - because we shouldn't be producing 
> thousands of errors to be consumed, because readers aren't going to read 
> thousands of errors, usually? Are your users/you reading all the output that 
> clang-tidy is generating?)

It effectively slows down the processing of files.

Same is there for stdout too.

For stderr messages, as you suggested, we may need option to switch off certain 
(stderr) messages.
Can you please suggest based on which condition we can switch it off?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90010

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

[PATCH] D90010: clang-tidy: Reduce number of stderr write calls

[Hiral via Phabricator via cfe-commits]

Hiralo added a comment.

In D90010#2355490 , @dblaikie wrote:

> By the looks of the code, you may want to call SetBufferSize only (do not 
> call SetBuffered after that - or it'll go back to the default buffer size of 
> 0.

Oh! I missed it!

 For example, consider clang-tidy running on 10,000 files... we expect to 
 have minimal number of write calls. With the code-as-is makes 10,000 * 7 = 
 70,000 stderr write calls with small small chunk of strings!!!
 With proposed changes OR with llvm::errs().setBuffered() set we will see 
 only 10,000 legitimate stderr write calls :)
>>>
>>> Right, but what I mean is "what observable difference do you see" - as a 
>>> user you aren't counting write calls, you're looking at the wall time, for 
>>> instance. What difference in user-observable behavior are you seeing/are 
>>> you trying to address, and how much does this change help that 
>>> user-observable behavior?
>>>
>>> I don't know too much about clang-tidy, but the statistics output for 
>>> 10,000 files doesn't seem like it'd be that useful, would it? So maybe the 
>>> solution is to turn it off, rather than to make it faster? (generally error 
>>> paths are thought of as not needing to be efficient - because we shouldn't 
>>> be producing thousands of errors to be consumed, because readers aren't 
>>> going to read thousands of errors, usually? Are your users/you reading all 
>>> the output that clang-tidy is generating?)
>>
>> It effectively slows down the processing of files.
>
> Do you have any measurements of how much it slows things down/how much the 
> patch you're proposing speeds things up?

Sorry, don't have stats now.

> In any case, I'd suggest either fixing the errors or, if you've decided the 
> errors are acceptable for some reason, using --quiet to silence the errors 
> you aren't interested in.

I think we tried with --quiet... but let me retry it... 
Meantime, we can pause review of this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90010

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

[PATCH] D89936: clang-tidy: adding "--clang-tidy-config=" to specify custom config file

[Hiral via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.h:68
+  /// Clang-tidy-config
+  llvm::Optional ClangTidyConfig;
+

DmitryPolukhin wrote:
> I'm not sure that we need it here. I would reuse code path for `--config` 
> options as much as possible and implement new option as a simple wrapper that 
> reads content of the file and interpret it as `--config` option. Moreover I 
> think it should not be possible to specify both options in command line.
> it should not be possible to specify both options in command line.

Made local changes with...

$ ./bin/clang-tidy -config="{Checks: '*'}" --config-file=/some/path/config --
Error: --config-file and --config are mutually exclusive. Specify only one.

$ ./bin/clang-tidy --checks='*' --config-file=/some/path/config --
Error: --config-file and --checks are mutually exclusive. Specify only one.




Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.h:233
   llvm::Optional tryReadConfigFile(llvm::StringRef Directory);
+  llvm::Optional tryReadConfigFile(llvm::StringRef Path,
+  bool IsFile);

DmitryPolukhin wrote:
> It looks like the second argument was added only for overload resolution. But 
> I think it is better to rename this function. After all it is not "try" 
> anymore because it reports fatal error in case of errors.
Renamed function to 'readConfigFile'.



Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:76
 
+static cl::opt ClangTidyConfig("clang-tidy-config", cl::desc(R"(
+Specify full path of .clang-tidy config file.

DmitryPolukhin wrote:
> I would call it something like `--config-file`
renamed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89936

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

[PATCH] D89936: clang-tidy: adding "--clang-tidy-config=" to specify custom config file

[Hiral via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp:324
+llvm::sys::fs::is_symlink_file(Twine(AbsoluteFilePath), IsLink);
+if (!(IsFile || IsLink)) {
+  std::string Msg;

DmitryPolukhin wrote:
> Is it actually required to check absolute path, link it or not, etc.? Why not 
> just try reading file with provided filename and report error if it fails?
Tried it and seems we don't need this changes since down the line below code 
emits required error message...

if (std::error_code EC = Text.getError()) {
 std::string Msg;
 llvm::raw_string_ostream ErrStream(Msg);
 ErrStream << " Can't read <" << Path << ">: " << EC.message() << "\n";
 llvm::report_fatal_error(ErrStream.str());
}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89936

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

[PATCH] D89936: clang-tidy: adding "--clang-tidy-config=" to specify custom config file

[Hiral via Phabricator via cfe-commits]

Hiralo updated this revision to Diff 300911.
Hiralo marked an inline comment as done.
Hiralo added a comment.

Incorporated review comments and updated patch.
(a) renamed var 'ClangTidyConfig' with 'ConfigFile'
(b) renamed function 'tryReadConfigFile(AbsoluteFilePath, true);' to 
'readConfigFile(ConfigPath)'
(c) renamed command-line option to '--config-file'
(d) made --config-file and --checks mutually exclusive
(e) make --config-file and --config mutually exclusive
(f) removed extra checks -- making absolute-path, isFile and isLink checks


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

https://reviews.llvm.org/D89936

Files:
  clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
  clang-tools-extra/clang-tidy/ClangTidyOptions.h
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp

Index: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -73,12 +73,13 @@
 )"),
cl::init(""), cl::cat(ClangTidyCategory));
 
-static cl::opt ClangTidyConfig("clang-tidy-config", cl::desc(R"(
-Specify full path of .clang-tidy config file.
-For example, --clang-tidy-config=/some/path/myTidyConfig
+static cl::opt ConfigFile("config-file", cl::desc(R"(
+Specify full path of .clang-tidy or custom config file.
+For example, --config-file=/some/path/myTidyConfig
+Note: this option is mutually exclusive to --config and --checks options.
 )"),
-cl::init(""),
-cl::cat(ClangTidyCategory));
+   cl::init(""),
+   cl::cat(ClangTidyCategory));
 
 static cl::opt WarningsAsErrors("warnings-as-errors", cl::desc(R"(
 Upgrades warnings to errors. Same format as
@@ -286,7 +287,7 @@
 
   ClangTidyOptions DefaultOptions;
   DefaultOptions.Checks = DefaultChecks;
-  DefaultOptions.ClangTidyConfig = "";
+  DefaultOptions.ConfigFile = "";
   DefaultOptions.WarningsAsErrors = "";
   DefaultOptions.HeaderFilterRegex = HeaderFilter;
   DefaultOptions.SystemHeaders = SystemHeaders;
@@ -299,8 +300,8 @@
   ClangTidyOptions OverrideOptions;
   if (Checks.getNumOccurrences() > 0)
 OverrideOptions.Checks = Checks;
-  if (ClangTidyConfig.getNumOccurrences() > 0)
-OverrideOptions.ClangTidyConfig = ClangTidyConfig;
+  if (ConfigFile.getNumOccurrences() > 0)
+OverrideOptions.ConfigFile = ConfigFile;
   if (WarningsAsErrors.getNumOccurrences() > 0)
 OverrideOptions.WarningsAsErrors = WarningsAsErrors;
   if (HeaderFilter.getNumOccurrences() > 0)
@@ -312,6 +313,19 @@
   if (UseColor.getNumOccurrences() > 0)
 OverrideOptions.UseColor = UseColor;
 
+  if (!ConfigFile.empty()) {
+if (!Config.empty()) {
+  llvm::errs() << "Error: --config-file and --config are mutually "
+  "exclusive. Specify only one.\n";
+  return nullptr;
+}
+if (!Checks.empty()) {
+  llvm::errs() << "Error: --config-file and --checks are mutually "
+  "exclusive. Specify only one.\n";
+  return nullptr;
+}
+  }
+
   if (!Config.empty()) {
 if (llvm::ErrorOr ParsedConfig =
 parseConfiguration(Config)) {
Index: clang-tools-extra/clang-tidy/ClangTidyOptions.h
===
--- clang-tools-extra/clang-tidy/ClangTidyOptions.h
+++ clang-tools-extra/clang-tidy/ClangTidyOptions.h
@@ -65,7 +65,7 @@
   llvm::Optional Checks;
 
   /// Clang-tidy-config
-  llvm::Optional ClangTidyConfig;
+  llvm::Optional ConfigFile;
 
   /// WarningsAsErrors filter.
   llvm::Optional WarningsAsErrors;
@@ -230,8 +230,10 @@
   /// Try to read configuration files from \p Directory using registered
   /// \c ConfigHandlers.
   llvm::Optional tryReadConfigFile(llvm::StringRef Directory);
-  llvm::Optional tryReadConfigFile(llvm::StringRef Path,
-  bool IsFile);
+
+  /// Try to read configuration files using registered
+  /// \c ConfigHandlers.
+  llvm::Optional readConfigFile(llvm::StringRef Path);
 
   llvm::StringMap CachedOptions;
   ClangTidyOptions OverrideOptions;
Index: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
@@ -109,7 +109,7 @@
 ClangTidyOptions ClangTidyOptions::getDefaults() {
   ClangTidyOptions Options;
   Options.Checks = "";
-  Options.ClangTidyConfig = "";
+  Options.ConfigFile = "";
   Options.WarningsAsErrors = "";
   Options.HeaderFilterRegex = "";
   Options.SystemHeaders = false;
@@ -307,42 +307,19 @@
   << "...\n");
   assert(FS && "FS must be set.");
 
-  llvm::SmallString<1024> AbsoluteFilePath(FileName);
- 

[PATCH] D89936: clang-tidy: adding "--clang-tidy-config=" to specify custom config file

[Hiral via Phabricator via cfe-commits]

Hiralo added a comment.

In D89936#2353048 , @DmitryPolukhin 
wrote:

> ...I think this diff needs some improvements

Thanks @DmitryPolukhin for your valuable comments/feedback.
Please review updated patch.

> + test for new option.

I am planning to add following test-case...

diff --git 
a/clang-tools-extra/test/clang-tidy/infrastructure/read_file_config.cpp 
b/clang-tools-extra/test/clang-tidy/infrastructure/read_file_config.cpp
...
 // RUN: clang-tidy -checks="-*,clang-analyzer-*" %T/read-file-config/test.cpp 
| grep "warning: .*\[clang-analyzer-deadcode.DeadStores\]$"
+// RUN: clang-tidy -config-file=%T/read-file-config/.clang-tidy 
%T/read-file-config/test.cpp | grep "warning: 
.*\[clang-analyzer-deadcode.DeadStores\]$"



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

https://reviews.llvm.org/D89936

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

[PATCH] D90010: clang-tidy: Reduce number of stderr write calls

[Hiral via Phabricator via cfe-commits]

Hiralo added a comment.

JFYI: using --quiet avoids call to following write calls...so that is useful.
write(2, "Suppressed ", 11) = 11
write(2, "10703", 5) = 5
write(2, " warnings (", 11) = 11
write(2, "10703", 5) = 5
write(2, " in non-user code", 17) = 17
write(2, ").\n", 3) = 3
write(2, "Use -header-filter=.* to display"..., 136) = 136

But still we have following stderr write calls, not sure which code emits...
write(2, "10712", 5) = 5
write(2, " warning", 8) = 8
write(2, "s", 1) = 1
write(2, " generated", 10) = 10
write(2, ".\n", 2) = 2


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90010

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

[PATCH] D90010: clang-tidy: Reduce number of stderr write calls

[Hiral via Phabricator via cfe-commits]

Hiralo added a comment.

Another observation w.r.t. stdout...

For example, consider following sample program:

#include 
#include 

using namespace std;
static long long A = 0ull;

void f(const std::string& a) {

  std::cout << a << std::endl;

}

int main() {
}


When running clang-tidy on this... we can see about 343 stdout write calls...
e.g. 
write(1, "\33[1m", 4)   = 4
write(1, "/some/path/a.cc", 14)  = 14
write(1, ":", 1)= 1
write(1, "4", 1)= 1
write(1, ":", 1)= 1
write(1, "1", 1)= 1
write(1, ":", 1)= 1
write(1, " ", 1)= 1
write(1, "\33[0m", 4)   = 4
write(1, "\33[0;1;35m", 9)  = 9
write(1, "warning", 7)  = 7
write(1, ": ", 2)   = 2
write(1, "\33[0m", 4)   = 4
write(1, "\33[1m", 4)   = 4
write(1, "do not use namespace using-direc"..., 100) = 100
write(1, "\33[0m", 4)   = 4
write(1, "\n", 1)   = 1
write(1, "using namespace std;", 20)= 20
write(1, "\n", 1)   = 1
write(1, "\33[0;1;32m", 9)  = 9
write(1, "^", 1)= 1
write(1, "\n", 1)   = 1
write(1, "\33[0m", 4)   = 4
write(1, "\33[1m", 4)   = 4
...

Shouldn't clang-tidy stdout with reasonable buffer-size (e.g. 128 or 512 or 
1024) ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90010

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

[PATCH] D89936: [clang-tidy] adding "--clang-tidy-config=" to specify custom config file

[Hiral via Phabricator via cfe-commits]

Hiralo added inline comments.



Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:330
   if (!Config.empty()) {
 if (llvm::ErrorOr ParsedConfig =
 parseConfiguration(Config)) {

DmitryPolukhin wrote:
> I think you can make this option much simpler if you just read file content 
> and use it or `Config` here. No changes in 
> clang-tools-extra/clang-tidy/ClangTidyOptions.h/.cpp will be required.
Sorry, didn't get. Can you please elaborate/pseudocode?



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

https://reviews.llvm.org/D89936

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

[PATCH] D89936: [clang-tidy] adding "--clang-tidy-config=" to specify custom config file

[Hiral via Phabricator via cfe-commits]

Hiralo added inline comments.



Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:330
   if (!Config.empty()) {
 if (llvm::ErrorOr ParsedConfig =
 parseConfiguration(Config)) {

Hiralo wrote:
> DmitryPolukhin wrote:
> > I think you can make this option much simpler if you just read file content 
> > and use it or `Config` here. No changes in 
> > clang-tools-extra/clang-tidy/ClangTidyOptions.h/.cpp will be required.
> Sorry, didn't get. Can you please elaborate/pseudocode?
> 
> I think you can make this option much simpler if you just read file content 
> and use it or `Config` here. No changes in 
> clang-tools-extra/clang-tidy/ClangTidyOptions.h/.cpp will be required.

Are you suggesting to...
(a) as per this patch have option '--config-file' (ClangTidyMain.cpp changes)
(b) read this file in ClangTidyMain.cpp itself and 
(c) pass YAML o/p to code-flow-of-config option (again change only in 
ClangTidyMain.cpp).

if so...

Shouldn't at call to tryReadConfigFile() kind of call in ClangTidyMain.cpp?

Can you please point me to LLVM/Clang File IO calls/examples?


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

https://reviews.llvm.org/D89936

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

[PATCH] D89936: [clang-tidy] adding "--clang-tidy-config=" to specify custom config file

[Hiral via Phabricator via cfe-commits]

Hiralo added inline comments.



Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:330
   if (!Config.empty()) {
 if (llvm::ErrorOr ParsedConfig =
 parseConfiguration(Config)) {

Hiralo wrote:
> Hiralo wrote:
> > DmitryPolukhin wrote:
> > > I think you can make this option much simpler if you just read file 
> > > content and use it or `Config` here. No changes in 
> > > clang-tools-extra/clang-tidy/ClangTidyOptions.h/.cpp will be required.
> > Sorry, didn't get. Can you please elaborate/pseudocode?
> > 
> > I think you can make this option much simpler if you just read file content 
> > and use it or `Config` here. No changes in 
> > clang-tools-extra/clang-tidy/ClangTidyOptions.h/.cpp will be required.
> 
> Are you suggesting to...
> (a) as per this patch have option '--config-file' (ClangTidyMain.cpp changes)
> (b) read this file in ClangTidyMain.cpp itself and 
> (c) pass YAML o/p to code-flow-of-config option (again change only in 
> ClangTidyMain.cpp).
> 
> if so...
> 
> Shouldn't at call to tryReadConfigFile() kind of call in ClangTidyMain.cpp?
> 
> Can you please point me to LLVM/Clang File IO calls/examples?
From code point of view... within ClangTidyMain.cpp itself... code-flow may 
look...

```
llvm::ErrorOr> Text = 
llvm::MemoryBuffer::getFile(ConfigFile.c_str());
if (std::error_code EC = Text.getError()) {
  llvm::errs() << "Can't read " << ConfigFile << ": " << EC.message() << "\n";
}

and then

Configassign(ConfigFile);
from then onwards it will be considered as Config values
```

Let me try implementing and test...


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

https://reviews.llvm.org/D89936

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

[PATCH] D89936: [clang-tidy] adding "--clang-tidy-config=" to specify custom config file

[Hiral via Phabricator via cfe-commits]

Hiralo updated this revision to Diff 300980.
Hiralo added a comment.

With updated patch now changes are only in ClangTidyMain.cpp.
Thanks to @DmitryPolukhin for valuable suggestion :)

Added '--config-file' option to specify custom config file.
ClangTidyMain.cpp reads ConfigFile into string and then assigned read data to 
'Config' i.e. makes like '--config' code flow internally.

Ref comments https://reviews.llvm.org/D89936#inline-839224

Thank you.
-Hiral


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

https://reviews.llvm.org/D89936

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


Index: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -73,6 +73,14 @@
 )"),
cl::init(""), cl::cat(ClangTidyCategory));
 
+static cl::opt ConfigFile("config-file", cl::desc(R"(
+Specify full path of .clang-tidy or custom config file.
+For example, --config-file=/some/path/myTidyConfig
+Note: this option is mutually exclusive to --config and --checks options.
+)"),
+   cl::init(""),
+   cl::cat(ClangTidyCategory));
+
 static cl::opt WarningsAsErrors("warnings-as-errors", cl::desc(R"(
 Upgrades warnings to errors. Same format as
 '-checks'.
@@ -279,6 +287,7 @@
 
   ClangTidyOptions DefaultOptions;
   DefaultOptions.Checks = DefaultChecks;
+  DefaultOptions.ConfigFile = "";
   DefaultOptions.WarningsAsErrors = "";
   DefaultOptions.HeaderFilterRegex = HeaderFilter;
   DefaultOptions.SystemHeaders = SystemHeaders;
@@ -291,6 +300,8 @@
   ClangTidyOptions OverrideOptions;
   if (Checks.getNumOccurrences() > 0)
 OverrideOptions.Checks = Checks;
+  if (ConfigFile.getNumOccurrences() > 0)
+OverrideOptions.ConfigFile = ConfigFile;
   if (WarningsAsErrors.getNumOccurrences() > 0)
 OverrideOptions.WarningsAsErrors = WarningsAsErrors;
   if (HeaderFilter.getNumOccurrences() > 0)
@@ -302,6 +313,30 @@
   if (UseColor.getNumOccurrences() > 0)
 OverrideOptions.UseColor = UseColor;
 
+  if (!ConfigFile.empty()) {
+if (!Config.empty()) {
+  llvm::errs() << "Error: --config-file and --config are mutually "
+  "exclusive. Specify only one.\n";
+  return nullptr;
+}
+
+if (!Checks.empty()) {
+  llvm::errs() << "Error: --config-file and --checks are mutually "
+  "exclusive. Specify only one.\n";
+  return nullptr;
+}
+
+llvm::ErrorOr> Text =
+llvm::MemoryBuffer::getFile(ConfigFile.c_str());
+if (std::error_code EC = Text.getError()) {
+  llvm::errs() << "Can't read config-file '" << ConfigFile
+   << "': " << EC.message() << "\n";
+  return nullptr;
+}
+
+Config.assign((*Text)->getBuffer());
+  }
+
   if (!Config.empty()) {
 if (llvm::ErrorOr ParsedConfig =
 parseConfiguration(Config)) {


Index: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -73,6 +73,14 @@
 )"),
cl::init(""), cl::cat(ClangTidyCategory));
 
+static cl::opt ConfigFile("config-file", cl::desc(R"(
+Specify full path of .clang-tidy or custom config file.
+For example, --config-file=/some/path/myTidyConfig
+Note: this option is mutually exclusive to --config and --checks options.
+)"),
+   cl::init(""),
+   cl::cat(ClangTidyCategory));
+
 static cl::opt WarningsAsErrors("warnings-as-errors", cl::desc(R"(
 Upgrades warnings to errors. Same format as
 '-checks'.
@@ -279,6 +287,7 @@
 
   ClangTidyOptions DefaultOptions;
   DefaultOptions.Checks = DefaultChecks;
+  DefaultOptions.ConfigFile = "";
   DefaultOptions.WarningsAsErrors = "";
   DefaultOptions.HeaderFilterRegex = HeaderFilter;
   DefaultOptions.SystemHeaders = SystemHeaders;
@@ -291,6 +300,8 @@
   ClangTidyOptions OverrideOptions;
   if (Checks.getNumOccurrences() > 0)
 OverrideOptions.Checks = Checks;
+  if (ConfigFile.getNumOccurrences() > 0)
+OverrideOptions.ConfigFile = ConfigFile;
   if (WarningsAsErrors.getNumOccurrences() > 0)
 OverrideOptions.WarningsAsErrors = WarningsAsErrors;
   if (HeaderFilter.getNumOccurrences() > 0)
@@ -302,6 +313,30 @@
   if (UseColor.getNumOccurrences() > 0)
 OverrideOptions.UseColor = UseColor;
 
+  if (!ConfigFile.empty()) {
+if (!Config.empty()) {
+  llvm::errs() << "Error: --config-file and --config are mutually "
+  "exclusive. Specify only one.\n";
+  return nullptr;
+}
+
+if (!Checks.empty()) {
+  llvm::errs() << "Error: --config-file and --checks are mutually "
+   

[PATCH] D89936: [clang-tidy] adding "--clang-tidy-config=" to specify custom config file

[Hiral via Phabricator via cfe-commits]

Hiralo updated this revision to Diff 301044.
Hiralo added a comment.

Added Lit test for --config-file cmdline option.


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

https://reviews.llvm.org/D89936

Files:
  clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
@@ -0,0 +1,12 @@
+// REQUIRES: shell
+// RUN: mkdir -p %T/config-file/
+// RUN: cp %s %T/read-file-config/test.cpp
+// RUN: echo 'Checks: "-*,hicpp-uppercase-literal-suffix"' > 
%T/read-file-config/.clang-tidy
+// RUN: clang-tidy --config-file=%T/read-file-config/.clang-tidy 
%T/read-file-config/test.cpp | grep "warning: 
.*\[hicpp-uppercase-literal-suffix\]$"
+
+void f() {
+  unsigned long c = 100ull;
+  int x;
+  x = 1;
+  x = 2;
+}


Index: clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
@@ -0,0 +1,12 @@
+// REQUIRES: shell
+// RUN: mkdir -p %T/config-file/
+// RUN: cp %s %T/read-file-config/test.cpp
+// RUN: echo 'Checks: "-*,hicpp-uppercase-literal-suffix"' > %T/read-file-config/.clang-tidy
+// RUN: clang-tidy --config-file=%T/read-file-config/.clang-tidy %T/read-file-config/test.cpp | grep "warning: .*\[hicpp-uppercase-literal-suffix\]$"
+
+void f() {
+  unsigned long c = 100ull;
+  int x;
+  x = 1;
+  x = 2;
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89936: [clang-tidy] adding "--clang-tidy-config=" to specify custom config file

[Hiral via Phabricator via cfe-commits]

Hiralo updated this revision to Diff 301047.
Hiralo added a comment.

Latest patch with lit test.


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

https://reviews.llvm.org/D89936

Files:
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
@@ -0,0 +1,12 @@
+// REQUIRES: shell
+// RUN: mkdir -p %T/config-file/
+// RUN: cp %s %T/read-file-config/test.cpp
+// RUN: echo 'Checks: "-*,hicpp-uppercase-literal-suffix"' > 
%T/read-file-config/.clang-tidy
+// RUN: clang-tidy --config-file=%T/read-file-config/.clang-tidy 
%T/read-file-config/test.cpp | grep "warning: 
.*\[hicpp-uppercase-literal-suffix\]$"
+
+void f() {
+  unsigned long c = 100ull;
+  int x;
+  x = 1;
+  x = 2;
+}
Index: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -73,6 +73,14 @@
 )"),
cl::init(""), cl::cat(ClangTidyCategory));
 
+static cl::opt ConfigFile("config-file", cl::desc(R"(
+Specify full path of .clang-tidy or custom config file.
+For example, --config-file=/some/path/myTidyConfig
+Note: this option is mutually exclusive to --config and --checks options.
+)"),
+   cl::init(""),
+   cl::cat(ClangTidyCategory));
+
 static cl::opt WarningsAsErrors("warnings-as-errors", cl::desc(R"(
 Upgrades warnings to errors. Same format as
 '-checks'.
@@ -279,6 +287,7 @@
 
   ClangTidyOptions DefaultOptions;
   DefaultOptions.Checks = DefaultChecks;
+  DefaultOptions.ConfigFile = "";
   DefaultOptions.WarningsAsErrors = "";
   DefaultOptions.HeaderFilterRegex = HeaderFilter;
   DefaultOptions.SystemHeaders = SystemHeaders;
@@ -291,6 +300,8 @@
   ClangTidyOptions OverrideOptions;
   if (Checks.getNumOccurrences() > 0)
 OverrideOptions.Checks = Checks;
+  if (ConfigFile.getNumOccurrences() > 0)
+OverrideOptions.ConfigFile = ConfigFile;
   if (WarningsAsErrors.getNumOccurrences() > 0)
 OverrideOptions.WarningsAsErrors = WarningsAsErrors;
   if (HeaderFilter.getNumOccurrences() > 0)
@@ -302,6 +313,30 @@
   if (UseColor.getNumOccurrences() > 0)
 OverrideOptions.UseColor = UseColor;
 
+  if (!ConfigFile.empty()) {
+if (!Config.empty()) {
+  llvm::errs() << "Error: --config-file and --config are mutually "
+  "exclusive. Specify only one.\n";
+  return nullptr;
+}
+
+if (!Checks.empty()) {
+  llvm::errs() << "Error: --config-file and --checks are mutually "
+  "exclusive. Specify only one.\n";
+  return nullptr;
+}
+
+llvm::ErrorOr> Text =
+llvm::MemoryBuffer::getFile(ConfigFile.c_str());
+if (std::error_code EC = Text.getError()) {
+  llvm::errs() << "Can't read config-file '" << ConfigFile
+   << "': " << EC.message() << "\n";
+  return nullptr;
+}
+
+Config.assign((*Text)->getBuffer());
+  }
+
   if (!Config.empty()) {
 if (llvm::ErrorOr ParsedConfig =
 parseConfiguration(Config)) {


Index: clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
@@ -0,0 +1,12 @@
+// REQUIRES: shell
+// RUN: mkdir -p %T/config-file/
+// RUN: cp %s %T/read-file-config/test.cpp
+// RUN: echo 'Checks: "-*,hicpp-uppercase-literal-suffix"' > %T/read-file-config/.clang-tidy
+// RUN: clang-tidy --config-file=%T/read-file-config/.clang-tidy %T/read-file-config/test.cpp | grep "warning: .*\[hicpp-uppercase-literal-suffix\]$"
+
+void f() {
+  unsigned long c = 100ull;
+  int x;
+  x = 1;
+  x = 2;
+}
Index: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -73,6 +73,14 @@
 )"),
cl::init(""), cl::cat(ClangTidyCategory));
 
+static cl::opt ConfigFile("config-file", cl::desc(R"(
+Specify full path of .clang-tidy or custom config file.
+For example, --config-file=/some/path/myTidyConfig
+Note: this option is mutually exclusive to --config and --checks options.
+)"),
+   cl::init(""),
+   cl::cat(ClangTidyCategory));
+
 static cl::opt WarningsAsErrors("warnings-as-errors", cl::desc(R"(
 Upgrades warnings to errors. Same format as
 '-checks'.
@@ -279,6 +287,7 @@
 
   ClangTidyOptions DefaultOptions;
 

[PATCH] D89936: [clang-tidy] adding "--clang-tidy-config=" to specify custom config file

[Hiral via Phabricator via cfe-commits]

Hiralo added a comment.

In D89936#2356583 , @DmitryPolukhin 
wrote:

> Yes, it is what I meant as a simpler solution. But please add Lit test even 
> for such trivial option.

Please review latest patch and Lit test-case.


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

https://reviews.llvm.org/D89936

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

[PATCH] D89936: [clang-tidy] adding "--clang-tidy-config=" to specify custom config file

[Hiral via Phabricator via cfe-commits]

Hiralo updated this revision to Diff 301065.
Hiralo added a comment.

Latest patch with Lit test-case.


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

https://reviews.llvm.org/D89936

Files:
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-file/config-file
  clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
@@ -0,0 +1,9 @@
+// REQUIRES: shell
+// RUN: clang-tidy --config-file=%S/Inputs/config-file/config-file 
%T/read-file-config/test.cpp | grep "warning: 
.*\[hicpp-uppercase-literal-suffix\]$"
+
+void f() {
+  unsigned long c = 100ull;
+  int x;
+  x = 1;
+  x = 2;
+}
Index: 
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-file/config-file
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-file/config-file
@@ -0,0 +1 @@
+Checks: "-*,hicpp-uppercase-literal-suffix"
Index: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -73,6 +73,14 @@
 )"),
cl::init(""), cl::cat(ClangTidyCategory));
 
+static cl::opt ConfigFile("config-file", cl::desc(R"(
+Specify full path of .clang-tidy or custom config file.
+For example, --config-file=/some/path/myTidyConfig
+Note: this option is mutually exclusive to --config and --checks options.
+)"),
+   cl::init(""),
+   cl::cat(ClangTidyCategory));
+
 static cl::opt WarningsAsErrors("warnings-as-errors", cl::desc(R"(
 Upgrades warnings to errors. Same format as
 '-checks'.
@@ -302,6 +310,30 @@
   if (UseColor.getNumOccurrences() > 0)
 OverrideOptions.UseColor = UseColor;
 
+  if (!ConfigFile.empty()) {
+if (!Config.empty()) {
+  llvm::errs() << "Error: --config-file and --config are mutually "
+  "exclusive. Specify only one.\n";
+  return nullptr;
+}
+
+if (!Checks.empty()) {
+  llvm::errs() << "Error: --config-file and --checks are mutually "
+  "exclusive. Specify only one.\n";
+  return nullptr;
+}
+
+llvm::ErrorOr> Text =
+llvm::MemoryBuffer::getFile(ConfigFile.c_str());
+if (std::error_code EC = Text.getError()) {
+  llvm::errs() << "Can't read config-file '" << ConfigFile
+   << "': " << EC.message() << "\n";
+  return nullptr;
+}
+
+Config.assign((*Text)->getBuffer());
+  }
+
   if (!Config.empty()) {
 if (llvm::ErrorOr ParsedConfig =
 parseConfiguration(Config)) {


Index: clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
@@ -0,0 +1,9 @@
+// REQUIRES: shell
+// RUN: clang-tidy --config-file=%S/Inputs/config-file/config-file %T/read-file-config/test.cpp | grep "warning: .*\[hicpp-uppercase-literal-suffix\]$"
+
+void f() {
+  unsigned long c = 100ull;
+  int x;
+  x = 1;
+  x = 2;
+}
Index: clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-file/config-file
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-file/config-file
@@ -0,0 +1 @@
+Checks: "-*,hicpp-uppercase-literal-suffix"
Index: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -73,6 +73,14 @@
 )"),
cl::init(""), cl::cat(ClangTidyCategory));
 
+static cl::opt ConfigFile("config-file", cl::desc(R"(
+Specify full path of .clang-tidy or custom config file.
+For example, --config-file=/some/path/myTidyConfig
+Note: this option is mutually exclusive to --config and --checks options.
+)"),
+   cl::init(""),
+   cl::cat(ClangTidyCategory));
+
 static cl::opt WarningsAsErrors("warnings-as-errors", cl::desc(R"(
 Upgrades warnings to errors. Same format as
 '-checks'.
@@ -302,6 +310,30 @@
   if (UseColor.getNumOccurrences() > 0)
 OverrideOptions.UseColor = UseColor;
 
+  if (!ConfigFile.empty()) {
+if (!Config.empty()) {
+  llvm::errs() << "Error: --config-file and --config are mutually "
+  "exclusive. Specify only one.\n";
+  return nullptr;
+}
+
+if (!Chec

[PATCH] D89936: [clang-tidy] adding "--clang-tidy-config=" to specify custom config file

[Hiral via Phabricator via cfe-commits]

Hiralo marked 4 inline comments as done.
Hiralo added inline comments.



Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:290
   DefaultOptions.Checks = DefaultChecks;
+  DefaultOptions.ConfigFile = "";
   DefaultOptions.WarningsAsErrors = "";

njames93 wrote:
> Given you've removed the references to `ConfigFile` in `ClangTidyOptions.h` 
> this should surely result in a compiler error, same goes below.
Thanks for heads-up and references.



Comment at: clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp:2
+// REQUIRES: shell
+// RUN: mkdir -p %T/config-file/
+// RUN: cp %s %T/read-file-config/test.cpp

DmitryPolukhin wrote:
> I think it is better to use some file in `%S/Inputs` like in 
> https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/test/clang-tidy/infrastructure/config-files.cpp
sure... in latest update added 
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-file/config-file


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

https://reviews.llvm.org/D89936

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

[PATCH] D89936: [clang-tidy] adding "--clang-tidy-config=" to specify custom config file

[Hiral via Phabricator via cfe-commits]

Hiralo added a comment.

In D89936#2357088 , @njames93 wrote:

> If you plan on contributing quite a lot then it would be wise to upload your 
> patches with arcanist - https://llvm.org/docs/Phabricator.html. It will help 
> prevent issues with diffs being relative to previous revisions.
> Personally I just create a branch from master for a feature. When I create 
> the patch or update, I just need to use `arc diff master` and it will handle 
> everything for me.
> The other advantage of using arc for patches is the pre-merge bot is then 
> able to build and check your patch to make sure all tests pass.

Sure thanks for info.

will there be any update in this review after build error or successful merge ?


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

https://reviews.llvm.org/D89936

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

[PATCH] D89936: [clang-tidy] adding "--clang-tidy-config=" to specify custom config file

[Hiral via Phabricator via cfe-commits]

Hiralo added inline comments.



Comment at: clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp:2
+// REQUIRES: shell
+// RUN: clang-tidy --config-file=%S/Inputs/config-file/config-file 
%T/read-file-config/test.cpp | grep "warning: 
.*\[hicpp-uppercase-literal-suffix\]$"
+

DmitryPolukhin wrote:
> Please use `FileCheck` as all other tests do instead of `grep` to make test 
> more portable. Also with `FileCheck`  it should be no need in `REQUIRES: 
> shell`.
> 
> And I think it is better to use `-dump-config` instead of checking specific 
> check on a source file. This way you will tests only functionality that you 
> implemented, without dependency some check behavior. Also it could be just 
> another tests case in clang-tidy/infrastructure/config-files.cpp.
> Please use `FileCheck` as all other tests do instead of `grep` to make test 
> more portable. Also with `FileCheck`  it should be no need in `REQUIRES: 
> shell`.
> 
> And I think it is better to use `-dump-config` instead of checking specific 
> check on a source file. This way you will tests only functionality that you 
> implemented, without dependency some check behavior. Also it could be just 
> another tests case in clang-tidy/infrastructure/config-files.cpp.

Sure...

Tried following...

```
// RUN: clang-tidy -config-file=%S/Inputs/config-file/config-file -dump-config 
-- | FileCheck %s -check-   prefix=CHECK-BASE
// CHECK-BASE: Checks: {{.*}}hicpp-uppercase-literal-suffix
// RUN: clang-tidy -config-file=%S/Inputs/config-file/config-file 
%S/config-file.cpp -- | FileCheck %s -check- prefix=CHECK-CHILD1
// CHECK-CHILD1: Checks: {{.*}}warning: integer literal has suffix 'ull', which 
is not uppercase [hicpp-uppercase- literal-suffix]  ### This one fails!
```

Can you please suggest how to match with following o/p:

```
path/clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp:7:21: 
warning: integer literal has suffix 'ull', which is not uppercase 
[hicpp-uppercase-literal-suffix]
  unsigned long c = 100ull;
^  ~~~
   ULL
```

JFYI: also tried matching just 'ULL'!


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

https://reviews.llvm.org/D89936

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

[PATCH] D89936: [clang-tidy] adding "--clang-tidy-config=" to specify custom config file

[Hiral via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp:2
+// REQUIRES: shell
+// RUN: clang-tidy --config-file=%S/Inputs/config-file/config-file 
%T/read-file-config/test.cpp | grep "warning: 
.*\[hicpp-uppercase-literal-suffix\]$"
+

Hiralo wrote:
> DmitryPolukhin wrote:
> > Please use `FileCheck` as all other tests do instead of `grep` to make test 
> > more portable. Also with `FileCheck`  it should be no need in `REQUIRES: 
> > shell`.
> > 
> > And I think it is better to use `-dump-config` instead of checking specific 
> > check on a source file. This way you will tests only functionality that you 
> > implemented, without dependency some check behavior. Also it could be just 
> > another tests case in clang-tidy/infrastructure/config-files.cpp.
> > Please use `FileCheck` as all other tests do instead of `grep` to make test 
> > more portable. Also with `FileCheck`  it should be no need in `REQUIRES: 
> > shell`.
> > 
> > And I think it is better to use `-dump-config` instead of checking specific 
> > check on a source file. This way you will tests only functionality that you 
> > implemented, without dependency some check behavior. Also it could be just 
> > another tests case in clang-tidy/infrastructure/config-files.cpp.
> 
> Sure...
> 
> Tried following...
> 
> ```
> // RUN: clang-tidy -config-file=%S/Inputs/config-file/config-file 
> -dump-config -- | FileCheck %s -check-   prefix=CHECK-BASE
> // CHECK-BASE: Checks: {{.*}}hicpp-uppercase-literal-suffix
> // RUN: clang-tidy -config-file=%S/Inputs/config-file/config-file 
> %S/config-file.cpp -- | FileCheck %s -check- prefix=CHECK-CHILD1
> // CHECK-CHILD1: Checks: {{.*}}warning: integer literal has suffix 'ull', 
> which is not uppercase [hicpp-uppercase- literal-suffix]  ### This 
> one fails!
> ```
> 
> Can you please suggest how to match with following o/p:
> 
> ```
> path/clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp:7:21: 
> warning: integer literal has suffix 'ull', which is not uppercase 
> [hicpp-uppercase-literal-suffix]
>   unsigned long c = 100ull;
> ^  ~~~
>ULL
> ```
> 
> JFYI: also tried matching just 'ULL'!
Removed below test...
// RUN: clang-tidy -config-file=%S/Inputs/config-file/config-file 
%S/config-file.cpp -- | FileCheck %s -check- prefix=CHECK-CHILD1



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

https://reviews.llvm.org/D89936

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

[PATCH] D89936: [clang-tidy] adding "--config-file=" to specify custom config file.

[Hiral via Phabricator via cfe-commits]

Hiralo updated this revision to Diff 301277.
Hiralo retitled this revision from "[clang-tidy] adding 
"--clang-tidy-config=" to specify custom config file" to 
"[clang-tidy] adding "--config-file=" to specify custom config 
file.".
Hiralo edited the summary of this revision.
Hiralo added a comment.

Latest patch with Lit test-case.

Updated Title and Summary.


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

https://reviews.llvm.org/D89936

Files:
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-file/config-file
  clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
@@ -0,0 +1,2 @@
+// RUN: clang-tidy -config-file=%S/Inputs/config-file/config-file -dump-config 
-- | FileCheck %s -check-prefix=CHECK-BASE
+// CHECK-BASE: Checks: {{.*}}hicpp-uppercase-literal-suffix
Index: 
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-file/config-file
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-file/config-file
@@ -0,0 +1 @@
+Checks: "-*,hicpp-uppercase-literal-suffix"
Index: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -73,6 +73,14 @@
 )"),
cl::init(""), cl::cat(ClangTidyCategory));
 
+static cl::opt ConfigFile("config-file", cl::desc(R"(
+Specify full path of .clang-tidy or custom config file.
+For example, --config-file=/some/path/myTidyConfig
+Note: this option is mutually exclusive to --config and --checks options.
+)"),
+   cl::init(""),
+   cl::cat(ClangTidyCategory));
+
 static cl::opt WarningsAsErrors("warnings-as-errors", cl::desc(R"(
 Upgrades warnings to errors. Same format as
 '-checks'.
@@ -302,6 +310,30 @@
   if (UseColor.getNumOccurrences() > 0)
 OverrideOptions.UseColor = UseColor;
 
+  if (!ConfigFile.empty()) {
+if (!Config.empty()) {
+  llvm::errs() << "Error: --config-file and --config are mutually "
+  "exclusive. Specify only one.\n";
+  return nullptr;
+}
+
+if (!Checks.empty()) {
+  llvm::errs() << "Error: --config-file and --checks are mutually "
+  "exclusive. Specify only one.\n";
+  return nullptr;
+}
+
+llvm::ErrorOr> Text =
+llvm::MemoryBuffer::getFile(ConfigFile.c_str());
+if (std::error_code EC = Text.getError()) {
+  llvm::errs() << "Can't read config-file '" << ConfigFile
+   << "': " << EC.message() << "\n";
+  return nullptr;
+}
+
+Config.assign((*Text)->getBuffer());
+  }
+
   if (!Config.empty()) {
 if (llvm::ErrorOr ParsedConfig =
 parseConfiguration(Config)) {


Index: clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
@@ -0,0 +1,2 @@
+// RUN: clang-tidy -config-file=%S/Inputs/config-file/config-file -dump-config -- | FileCheck %s -check-prefix=CHECK-BASE
+// CHECK-BASE: Checks: {{.*}}hicpp-uppercase-literal-suffix
Index: clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-file/config-file
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-file/config-file
@@ -0,0 +1 @@
+Checks: "-*,hicpp-uppercase-literal-suffix"
Index: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -73,6 +73,14 @@
 )"),
cl::init(""), cl::cat(ClangTidyCategory));
 
+static cl::opt ConfigFile("config-file", cl::desc(R"(
+Specify full path of .clang-tidy or custom config file.
+For example, --config-file=/some/path/myTidyConfig
+Note: this option is mutually exclusive to --config and --checks options.
+)"),
+   cl::init(""),
+   cl::cat(ClangTidyCategory));
+
 static cl::opt WarningsAsErrors("warnings-as-errors", cl::desc(R"(
 Upgrades warnings to errors. Same format as
 '-checks'.
@@ -302,6 +310,30 @@
   if (UseColor.getNumOccurrences() > 0)
 OverrideOptions.UseColor = UseColor;
 
+  if (!ConfigFile.empty()) {
+if (!Config.empty()) {
+  llvm::errs() << "Error: --config-file and

[PATCH] D89936: [clang-tidy] adding "--config-file=" to specify custom config file.

[Hiral via Phabricator via cfe-commits]

Hiralo added a comment.

In D89936#2358572 , @DmitryPolukhin 
wrote:

> I think this diff looks very close to what we need. I hope it will be the 
> last iteration.
> Please also update title and description with the new name of the option, etc.

Updated latest patch :)


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

https://reviews.llvm.org/D89936

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

[PATCH] D89936: [clang-tidy] adding "--config-file=" to specify custom config file.

[Hiral via Phabricator via cfe-commits]

Hiralo added inline comments.



Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:337
+
+Config.assign((*Text)->getBuffer());
+  }

DmitryPolukhin wrote:
> DmitryPolukhin wrote:
> > I suggest creating new local variable with text of the config from `Config` 
> > or `ConfigFile` file content i.e. avoid modifying `Config` itself.
> It doesn't compile and using local variable instead of changing command line 
> option will make this code easier to read and understand:
> ```
> clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:334:12: error: no 
> matching member function for call to 'assign'
> Config.assign((*Text)->getBuffer());
> ~~~^~
> ```
> Could you please fix build and please upload diff with `arc diff` as 
> @njames93 suggested so buildbot can test your changes?
> I suggest creating new local variable with text of the config from `Config` 
> or `ConfigFile` file content i.e. avoid modifying `Config` itself.

Sorry! probably I missed this comment!

std::string Temp = (*Text)->getBuffer();
Config.assign(Temp); // ???

if so how code will enter into below block... because we want to reuse 
code-path of 'Config' correct?

if (!Config.empty()) {
 if (llvm::ErrorOr ParsedConfig = parseConfiguration(Config)) 
{
...
}

?


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

https://reviews.llvm.org/D89936

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

[PATCH] D89936: [clang-tidy] adding "--config-file=" to specify custom config file.

[Hiral via Phabricator via cfe-commits]

Hiralo added inline comments.



Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:320-324
+if (!Checks.empty()) {
+  llvm::errs() << "Error: --config-file and --checks are mutually "
+  "exclusive. Specify only one.\n";
+  return nullptr;
+}

DmitryPolukhin wrote:
> DmitryPolukhin wrote:
> > njames93 wrote:
> > > I disagree with this check here, `Config` is not mutually exclusive with 
> > > `Checks`, `Checks` gets applied atop of `Config`. So the same should 
> > > happen when using `ConfigFile` with `Checks`
> > +1
> Clarify: +1 to @njames93 that we don't need this check.
> I disagree with this check here, `Config` is not mutually exclusive with 
> `Checks`, `Checks` gets applied atop of `Config`. So the same should happen 
> when using `ConfigFile` with `Checks`

ok, will remove these two checks :)



Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:337
+
+Config.assign((*Text)->getBuffer());
+  }

Hiralo wrote:
> DmitryPolukhin wrote:
> > DmitryPolukhin wrote:
> > > I suggest creating new local variable with text of the config from 
> > > `Config` or `ConfigFile` file content i.e. avoid modifying `Config` 
> > > itself.
> > It doesn't compile and using local variable instead of changing command 
> > line option will make this code easier to read and understand:
> > ```
> > clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:334:12: error: no 
> > matching member function for call to 'assign'
> > Config.assign((*Text)->getBuffer());
> > ~~~^~
> > ```
> > Could you please fix build and please upload diff with `arc diff` as 
> > @njames93 suggested so buildbot can test your changes?
> > I suggest creating new local variable with text of the config from `Config` 
> > or `ConfigFile` file content i.e. avoid modifying `Config` itself.
> 
> Sorry! probably I missed this comment!
> 
> std::string Temp = (*Text)->getBuffer();
> Config.assign(Temp); // ???
> 
> if so how code will enter into below block... because we want to reuse 
> code-path of 'Config' correct?
> 
> if (!Config.empty()) {
>  if (llvm::ErrorOr ParsedConfig = 
> parseConfiguration(Config)) {
> ...
> }
> 
> ?
> It doesn't compile and using local variable instead of changing command line 
> option will make this code easier to read and understand:
> ```
> clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:334:12: error: no 
> matching member function for call to 'assign'
> Config.assign((*Text)->getBuffer());
> ~~~^~
> ```
> Could you please fix build and please upload diff with `arc diff` as 
> @njames93 suggested so buildbot can test your changes?

Let me try.


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

https://reviews.llvm.org/D89936

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

[PATCH] D89936: [clang-tidy] adding "--config-file=" to specify custom config file.

[Hiral via Phabricator via cfe-commits]

Hiralo updated this revision to Diff 301845.
Hiralo added a comment.

clang-tidy: adding "--config-file=" to specify custom config file.

Let clang-tidy to read config from specified file.

This option internally works exactly the same way as --config option
after reading specified config file.

This option works as helper for --config in following few cases:

- when there is shell limitation to read file content to the command line,
- when there is limitation to command line length,
- setting right build dependencies, and
- others

--config-file and --config options are made mutually exclusive.

Example usage:
 $ clang-tidy --config-file=/some/path/myTidyConfigFile --dump-config --
 ...this will read config from /some/path/myTidyConfigFile and

  avoid searching '.clang-tidy' in parent-dirs.

May speed-up tidy runtime since now it will just look-up 
instead of searching ".clang-tidy" in parent-dirs.

Reviewed By: DmitryPolukhin, njames93

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89936

Files:
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-file/config-file
  clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
@@ -0,0 +1,2 @@
+// RUN: clang-tidy -config-file=%S/Inputs/config-file/config-file -dump-config 
-- | FileCheck %s -check-prefix=CHECK-BASE
+// CHECK-BASE: Checks: {{.*}}hicpp-uppercase-literal-suffix
Index: 
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-file/config-file
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-file/config-file
@@ -0,0 +1 @@
+Checks: "-*,hicpp-uppercase-literal-suffix"
Index: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -168,6 +168,16 @@
 )"),
cl::init(""), cl::cat(ClangTidyCategory));
 
+static cl::opt ConfigFile("config-file", cl::desc(R"(
+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.
+)"),
+   cl::init(""),
+   cl::cat(ClangTidyCategory));
+
 static cl::opt DumpConfig("dump-config", cl::desc(R"(
 Dumps configuration in the YAML format to
 stdout. This option can be used along with a
@@ -302,19 +312,41 @@
   if (UseColor.getNumOccurrences() > 0)
 OverrideOptions.UseColor = UseColor;
 
-  if (!Config.empty()) {
-if (llvm::ErrorOr ParsedConfig =
-parseConfiguration(Config)) {
+  auto LoadConfig = [&](StringRef Configuration)
+  -> std::unique_ptr {
+llvm::ErrorOr ParsedConfig =
+parseConfiguration(Configuration);
+if (ParsedConfig)
   return std::make_unique(
   GlobalOptions,
   ClangTidyOptions::getDefaults().mergeWith(DefaultOptions, 0),
   *ParsedConfig, OverrideOptions, std::move(FS));
-} else {
-  llvm::errs() << "Error: invalid configuration specified.\n"
-   << ParsedConfig.getError().message() << "\n";
+llvm::errs() << "Error: invalid configuration specified.\n"
+ << ParsedConfig.getError().message() << "\n";
+return nullptr;
+  };
+
+  if (!ConfigFile.empty()) {
+if (!Config.empty()) {
+  llvm::errs() << "Error: --config-file and --config are "
+  "mutually exclusive. Specify only one.\n";
   return nullptr;
 }
+
+llvm::ErrorOr> Text =
+llvm::MemoryBuffer::getFile(ConfigFile.c_str());
+if (std::error_code EC = Text.getError()) {
+  llvm::errs() << "Error: can't read config-file '" << ConfigFile
+   << "': " << EC.message() << "\n";
+  return nullptr;
+}
+
+return LoadConfig((*Text)->getBuffer());
   }
+
+  if (!Config.empty())
+return LoadConfig(Config);
+
   return std::make_unique(GlobalOptions, DefaultOptions,
 OverrideOptions, 
std::move(FS));
 }


Index: clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
@@ -0,0 +1,2 @@
+// RUN: clang-tidy -config-file=%S/Inputs/config-file/config-fi

[PATCH] D89936: [clang-tidy] adding "--config-file=" to specify custom config file.

[Hiral via Phabricator via cfe-commits]

Hiralo marked 6 inline comments as done.
Hiralo added inline comments.



Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:320-324
+if (!Checks.empty()) {
+  llvm::errs() << "Error: --config-file and --checks are mutually "
+  "exclusive. Specify only one.\n";
+  return nullptr;
+}

DmitryPolukhin wrote:
> Hiralo wrote:
> > DmitryPolukhin wrote:
> > > DmitryPolukhin wrote:
> > > > njames93 wrote:
> > > > > I disagree with this check here, `Config` is not mutually exclusive 
> > > > > with `Checks`, `Checks` gets applied atop of `Config`. So the same 
> > > > > should happen when using `ConfigFile` with `Checks`
> > > > +1
> > > Clarify: +1 to @njames93 that we don't need this check.
> > > I disagree with this check here, `Config` is not mutually exclusive with 
> > > `Checks`, `Checks` gets applied atop of `Config`. So the same should 
> > > happen when using `ConfigFile` with `Checks`
> > 
> > ok, will remove these two checks :)
> I think we need to keep the first check. There is no reason to specify both 
> `--config` and `--config-file` or give on of them precedence.
> I think we need to keep the first check. There is no reason to specify both 
> `--config` and `--config-file` or give on of them precedence.

Kept '--config-file' and '--config' mutually exclusive.





Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:337
+
+Config.assign((*Text)->getBuffer());
+  }

njames93 wrote:
> Hiralo wrote:
> > Hiralo wrote:
> > > DmitryPolukhin wrote:
> > > > DmitryPolukhin wrote:
> > > > > I suggest creating new local variable with text of the config from 
> > > > > `Config` or `ConfigFile` file content i.e. avoid modifying `Config` 
> > > > > itself.
> > > > It doesn't compile and using local variable instead of changing command 
> > > > line option will make this code easier to read and understand:
> > > > ```
> > > > clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:334:12: error: no 
> > > > matching member function for call to 'assign'
> > > > Config.assign((*Text)->getBuffer());
> > > > ~~~^~
> > > > ```
> > > > Could you please fix build and please upload diff with `arc diff` as 
> > > > @njames93 suggested so buildbot can test your changes?
> > > > I suggest creating new local variable with text of the config from 
> > > > `Config` or `ConfigFile` file content i.e. avoid modifying `Config` 
> > > > itself.
> > > 
> > > Sorry! probably I missed this comment!
> > > 
> > > std::string Temp = (*Text)->getBuffer();
> > > Config.assign(Temp); // ???
> > > 
> > > if so how code will enter into below block... because we want to reuse 
> > > code-path of 'Config' correct?
> > > 
> > > if (!Config.empty()) {
> > >  if (llvm::ErrorOr ParsedConfig = 
> > > parseConfiguration(Config)) {
> > > ...
> > > }
> > > 
> > > ?
> > > It doesn't compile and using local variable instead of changing command 
> > > line option will make this code easier to read and understand:
> > > ```
> > > clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:334:12: error: no 
> > > matching member function for call to 'assign'
> > > Config.assign((*Text)->getBuffer());
> > > ~~~^~
> > > ```
> > > Could you please fix build and please upload diff with `arc diff` as 
> > > @njames93 suggested so buildbot can test your changes?
> > 
> > Let me try.
> Could extract the configuration loading into a lambda
> ```
>   auto LoadConfig = [&](StringRef Configuration)
>   -> std::unique_ptr {
> llvm::ErrorOr ParsedConfig =
> parseConfiguration(Configuration);
> if (ParsedConfig)
>   return std::make_unique(
>   GlobalOptions,
>   ClangTidyOptions::getDefaults().mergeWith(DefaultOptions, 0),
>   *ParsedConfig, OverrideOptions, std::move(FS));
> llvm::errs() << "Error: invalid configuration specified.\n"
>  << ParsedConfig.getError().message() << "\n";
> return nullptr;
>   };
> ```
> 
> Then further down
> ```
> if (!ConfigFile.empty()) {
>   ...
>   llvm::ErrorOr> Text =
> llvm::MemoryBuffer::getFile(ConfigFile.c_str());
> if (std::error_code EC = Text.getError()) {
>   llvm::errs() << "Can't read config-file '" << ConfigFile
><< "': " << EC.message() << "\n";
>   return nullptr;
> }
> 
> return LoadConfig((*Text)->getBuffer());
> }
> 
> if (!Config.empty())
>   return LoadConfig(Config);
> ```
> 
> This has the added bonus of not having to needlessly copy the buffer
> > It doesn't compile and using local variable instead of changing command 
> > line option will make this code easier to read and understand:
> > ```
> > clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:334:12: error: no 
> > matching member function for call to 'assign'
> > Config.assign((*Text)->getBuffer());
> > ~~~^~
> > ```
> > Could you please fix build and please upload diff with `a

[PATCH] D89936: [clang-tidy] adding "--config-file=" to specify custom config file.

[Hiral via Phabricator via cfe-commits]

Hiralo added inline comments.



Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:329-330
+
+  if (!ConfigFile.empty()) {
+if (!Config.empty()) {
+  llvm::errs() << "Error: --config-file and --config are "

njames93 wrote:
> nit: Should we be using `Config(File)?.getNumOccurances() > 0` here and below
> If someone puts `--config=""` They are specifying a config(file)?, even if 
> its empty.
> nit: Should we be using `Config(File)?.getNumOccurances() > 0` here and below
> If someone puts `--config=""` They are specifying a config(file)?, even if 
> its empty.

Sounds good.

What is expected o/p with -config="" ? 

Currently it works as below...
$ ./myinstall/bin/clang-tidy -config="" --dump-config --
config_occurances =1
config_is_empty = yes


```
Checks:  'clang-diagnostic-*,clang-analyzer-*'

WarningsAsErrors: ''
HeaderFilterRegex: ''
AnalyzeTemporaryDtors: false
FormatStyle: none
CheckOptions:
  - key: llvm-else-after-return.WarnOnConditionVariables
...
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89936

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

[PATCH] D89936: [clang-tidy] adding "--config-file=" to specify custom config file.

[Hiral via Phabricator via cfe-commits]

Hiralo updated this revision to Diff 301862.
Hiralo added a comment.

clang-tidy: adding "--config-file=" to specify custom config file.

Let clang-tidy to read config from specified file.

This option internally works exactly the same way as --config option
after reading specified config file.

This option works as helper for --config in following few cases:

- when there is shell limitation to read file content to the command line,
- when there is limitation to command line length,
- setting right build dependencies, and
- others

--config-file and --config options are made mutually exclusive.

Example usage:
$ clang-tidy --config-file=/some/path/myTidyConfigFile --dump-config --
...this will read config from /some/path/myTidyConfigFile and

  avoid searching '.clang-tidy' in parent-dirs.

May speed-up tidy runtime since now it will just look-up 
instead of searching ".clang-tidy" in parent-dirs.

Reviewed By: DmitryPolukhin, njames93

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89936

Files:
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-file/config-file
  clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
@@ -0,0 +1,2 @@
+// RUN: clang-tidy -config-file=%S/Inputs/config-file/config-file -dump-config 
-- | FileCheck %s -check-prefix=CHECK-BASE
+// CHECK-BASE: Checks: {{.*}}hicpp-uppercase-literal-suffix
Index: 
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-file/config-file
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/config-file/config-file
@@ -0,0 +1 @@
+Checks: "-*,hicpp-uppercase-literal-suffix"
Index: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -168,6 +168,16 @@
 )"),
cl::init(""), cl::cat(ClangTidyCategory));
 
+static cl::opt ConfigFile("config-file", cl::desc(R"(
+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.
+)"),
+   cl::init(""),
+   cl::cat(ClangTidyCategory));
+
 static cl::opt DumpConfig("dump-config", cl::desc(R"(
 Dumps configuration in the YAML format to
 stdout. This option can be used along with a
@@ -302,19 +312,41 @@
   if (UseColor.getNumOccurrences() > 0)
 OverrideOptions.UseColor = UseColor;
 
-  if (!Config.empty()) {
-if (llvm::ErrorOr ParsedConfig =
-parseConfiguration(Config)) {
+  auto LoadConfig = [&](StringRef Configuration)
+  -> std::unique_ptr {
+llvm::ErrorOr ParsedConfig =
+parseConfiguration(Configuration);
+if (ParsedConfig)
   return std::make_unique(
   GlobalOptions,
   ClangTidyOptions::getDefaults().mergeWith(DefaultOptions, 0),
   *ParsedConfig, OverrideOptions, std::move(FS));
-} else {
-  llvm::errs() << "Error: invalid configuration specified.\n"
-   << ParsedConfig.getError().message() << "\n";
+llvm::errs() << "Error: invalid configuration specified.\n"
+ << ParsedConfig.getError().message() << "\n";
+return nullptr;
+  };
+
+  if (ConfigFile.getNumOccurrences() > 0) {
+if (Config.getNumOccurrences() > 0) {
+  llvm::errs() << "Error: --config-file and --config are "
+  "mutually exclusive. Specify only one.\n";
   return nullptr;
 }
+
+llvm::ErrorOr> Text =
+llvm::MemoryBuffer::getFile(ConfigFile.c_str());
+if (std::error_code EC = Text.getError()) {
+  llvm::errs() << "Error: can't read config-file '" << ConfigFile
+   << "': " << EC.message() << "\n";
+  return nullptr;
+}
+
+return LoadConfig((*Text)->getBuffer());
   }
+
+  if (Config.getNumOccurrences() > 0)
+return LoadConfig(Config);
+
   return std::make_unique(GlobalOptions, DefaultOptions,
 OverrideOptions, 
std::move(FS));
 }


Index: clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/config-file.cpp
@@ -0,0 +1,2 @@
+// RUN: clang-tidy -

[PATCH] D89936: [clang-tidy] adding "--config-file=" to specify custom config file.

[Hiral via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:329-330
+
+  if (!ConfigFile.empty()) {
+if (!Config.empty()) {
+  llvm::errs() << "Error: --config-file and --config are "

Hiralo wrote:
> njames93 wrote:
> > nit: Should we be using `Config(File)?.getNumOccurances() > 0` here and 
> > below
> > If someone puts `--config=""` They are specifying a config(file)?, even if 
> > its empty.
> > nit: Should we be using `Config(File)?.getNumOccurances() > 0` here and 
> > below
> > If someone puts `--config=""` They are specifying a config(file)?, even if 
> > its empty.
> 
> Sounds good.
> 
> What is expected o/p with -config="" ? 
> 
> Currently it works as below...
> $ ./myinstall/bin/clang-tidy -config="" --dump-config --
> config_occurances =1
> config_is_empty = yes
> 
> 
> ```
> Checks:  'clang-diagnostic-*,clang-analyzer-*'
> 
> WarningsAsErrors: ''
> HeaderFilterRegex: ''
> AnalyzeTemporaryDtors: false
> FormatStyle: none
> CheckOptions:
>   - key: llvm-else-after-return.WarnOnConditionVariables
> ...
> ```
> nit: Should we be using `Config(File)?.getNumOccurances() > 0` here and below
> If someone puts `--config=""` They are specifying a config(file)?, even if 
> its empty.

Updated patch...

$ ./myinstall/bin/clang-tidy --config="" --config-file="" --dump-config --
Error: --config-file and --config are mutually exclusive. Specify only one.

Thanks,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89936

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

[PATCH] D89936: [clang-tidy] adding "--config-file=" to specify custom config file.

[Hiral via Phabricator via cfe-commits]

Hiralo added inline comments.



Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:329-330
+
+  if (!ConfigFile.empty()) {
+if (!Config.empty()) {
+  llvm::errs() << "Error: --config-file and --config are "

Hiralo wrote:
> Hiralo wrote:
> > njames93 wrote:
> > > nit: Should we be using `Config(File)?.getNumOccurances() > 0` here and 
> > > below
> > > If someone puts `--config=""` They are specifying a config(file)?, even 
> > > if its empty.
> > > nit: Should we be using `Config(File)?.getNumOccurances() > 0` here and 
> > > below
> > > If someone puts `--config=""` They are specifying a config(file)?, even 
> > > if its empty.
> > 
> > Sounds good.
> > 
> > What is expected o/p with -config="" ? 
> > 
> > Currently it works as below...
> > $ ./myinstall/bin/clang-tidy -config="" --dump-config --
> > config_occurances =1
> > config_is_empty = yes
> > 
> > 
> > ```
> > Checks:  'clang-diagnostic-*,clang-analyzer-*'
> > 
> > WarningsAsErrors: ''
> > HeaderFilterRegex: ''
> > AnalyzeTemporaryDtors: false
> > FormatStyle: none
> > CheckOptions:
> >   - key: llvm-else-after-return.WarnOnConditionVariables
> > ...
> > ```
> > nit: Should we be using `Config(File)?.getNumOccurances() > 0` here and 
> > below
> > If someone puts `--config=""` They are specifying a config(file)?, even if 
> > its empty.
> 
> Updated patch...
> 
> $ ./myinstall/bin/clang-tidy --config="" --config-file="" --dump-config --
> Error: --config-file and --config are mutually exclusive. Specify only one.
> 
> Thanks,
> > nit: Should we be using `Config(File)?.getNumOccurances() > 0` here and 
> > below
> > If someone puts `--config=""` They are specifying a config(file)?, even if 
> > its empty.
> 
> Updated patch...
> 
> $ ./myinstall/bin/clang-tidy --config="" --config-file="" --dump-config --
> Error: --config-file and --config are mutually exclusive. Specify only one.
> 
> Thanks,

and

$ ./myinstall/bin/clang-tidy --config-file="" --dump-config --
Error: can't read config-file '': No such file or directory




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89936

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

[PATCH] D89936: [clang-tidy] adding "--config-file=" to specify custom config file.

[Hiral via Phabricator via cfe-commits]

Hiralo added a comment.

In D89936#2364665 , @DmitryPolukhin 
wrote:

> Looks good to me.

Thanks @DmitryPolukhin and @njames93 for your help.

> This revision is now accepted and ready to land.Fri, Oct 30, 8:57 PM

When this patch will be merged and available in master?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89936

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

[PATCH] D89936: [clang-tidy] adding "--config-file=" to specify custom config file.

[Hiral via Phabricator via cfe-commits]

Hiralo added a comment.

In D89936#2370712 , @DmitryPolukhin 
wrote:

> In D89936#2370163 , @Hiralo wrote:
>
>> When this patch will be merged and available in master?
>
> @Hiralo I can push this changes to master for you. Please let me know if you 
> need it.

Yes please.

And just for confirmation, this patch will be part of the release 12.0.0 (i.e. 
Dec release), correct?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89936

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

[PATCH] D89936: [clang-tidy] adding "--config-file=" to specify custom config file.

[Hiral via Phabricator via cfe-commits]

Hiralo added a comment.

Hello @@DmitryPolukhin ,

When I submitted latest via 'arc diff' my commit-message was...
Can you please help to have following commit message? Below commit message is 
more clear and helpful.
Sorry for inconvenience caused!

commit 653cb7912bbe4daabc8d6dd6dca71b2cf9a10365 (HEAD -> master)
Author: Hiral Oza 
Date:   Fri Oct 30 05:07:19 2020 -0400

  clang-tidy: adding "--config-file=" to specify custom config file.
  
  Let clang-tidy to read config from specified file.
  
  This option internally works exactly the same way as --config option
  after reading specified config file.
  
  This option works as helper for --config in following few cases:
   - when there is shell limitation to read file content to the command line,
   - when there is limitation to command line length,
   - setting right build dependencies, and
   - others
  
  --config-file and --config options are made mutually exclusive.
  
  Example usage:
   $ clang-tidy --config-file=/some/path/myTidyConfigFile --dump-config --
   ...this will read config from /some/path/myTidyConfigFile and
  avoid searching '.clang-tidy' in parent-dirs.
  
  May speed-up tidy runtime since now it will just look-up 
  instead of searching ".clang-tidy" in parent-dirs.
  
  Reviewed By: DmitryPolukhin, njames93
  
  Differential Revision: https://reviews.llvm.org/D89936


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89936

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