From 54c65d5fb22c3cc53ecd707687c2f25b1643726b Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Sun, 12 Jun 2016 22:06:01 +0200 Subject: [PATCH 1/2] cmake: Extract DisplayMessage API. --- Source/cmake.cxx | 6 ++++++ Source/cmake.h | 3 +++ 2 files changed, 9 insertions(+) diff --git a/Source/cmake.cxx b/Source/cmake.cxx index ecbdc61a8..657091b42 100644 --- a/Source/cmake.cxx +++ b/Source/cmake.cxx @@ -2310,6 +2310,12 @@ void cmake::IssueMessage(cmake::MessageType t, std::string const& text, return; } + this->DisplayMessage(t, text, backtrace); +} + +void cmake::DisplayMessage(cmake::MessageType t, std::string const& text, + cmListFileBacktrace const& backtrace) const +{ std::ostringstream msg; if (!printMessagePreamble(t, msg)) { return; diff --git a/Source/cmake.h b/Source/cmake.h index 4958a05dc..1d63ede80 100644 --- a/Source/cmake.h +++ b/Source/cmake.h @@ -384,6 +384,9 @@ public: cmListFileBacktrace const& backtrace = cmListFileBacktrace(), bool force = false) const; + void DisplayMessage(cmake::MessageType t, std::string const& text, + cmListFileBacktrace const& backtrace) const; + ///! run the --build option int Build(const std::string& dir, const std::string& target, const std::string& config, From 23f87e8157770c56d3aa568f3d1318f9b9070364 Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Thu, 9 Jun 2016 09:57:47 +0200 Subject: [PATCH 2/2] cmake: Remove force from IssueMessage API The force parameter is ugly and makes the method harder to reason about (issues the message ... but maybe it doesn't ... but then again you can force it). It is a violation of https://en.wikipedia.org/wiki/Interface_segregation_principle and is the kind of thing described in a recent blog here: http://code.joejag.com/2016/anti-if-the-missing-patterns.html "Any time you see this you actually have two methods bundled into one. That boolean represents an opportunity to name a concept in your code." --- Source/cmMakefile.cxx | 6 +++--- Source/cmMakefile.h | 3 +-- Source/cmMessageCommand.cxx | 5 +++-- Source/cmake.cxx | 16 +++++++--------- Source/cmake.h | 3 +-- 5 files changed, 15 insertions(+), 18 deletions(-) diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index ca30b3d66..00ff5ac8c 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -105,8 +105,8 @@ cmMakefile::~cmMakefile() cmDeleteAll(this->EvaluationFiles); } -void cmMakefile::IssueMessage(cmake::MessageType t, std::string const& text, - bool force) const +void cmMakefile::IssueMessage(cmake::MessageType t, + std::string const& text) const { // Collect context information. if (!this->ExecutionStatusStack.empty()) { @@ -114,7 +114,7 @@ void cmMakefile::IssueMessage(cmake::MessageType t, std::string const& text, this->ExecutionStatusStack.back()->SetNestedError(true); } } - this->GetCMakeInstance()->IssueMessage(t, text, this->GetBacktrace(), force); + this->GetCMakeInstance()->IssueMessage(t, text, this->GetBacktrace()); } cmStringRange cmMakefile::GetIncludeDirectoriesEntries() const diff --git a/Source/cmMakefile.h b/Source/cmMakefile.h index c665b1f12..719c7648f 100644 --- a/Source/cmMakefile.h +++ b/Source/cmMakefile.h @@ -715,8 +715,7 @@ public: cmMakefile* Makefile; }; - void IssueMessage(cmake::MessageType t, std::string const& text, - bool force = false) const; + void IssueMessage(cmake::MessageType t, std::string const& text) const; /** Set whether or not to report a CMP0000 violation. */ void SetCheckCMP0000(bool b) { this->CheckCMP0000 = b; } diff --git a/Source/cmMessageCommand.cxx b/Source/cmMessageCommand.cxx index f4458a7da..689e3fa8d 100644 --- a/Source/cmMessageCommand.cxx +++ b/Source/cmMessageCommand.cxx @@ -63,8 +63,9 @@ bool cmMessageCommand::InitialPass(std::vector const& args, std::string message = cmJoin(cmMakeRange(i, args.end()), std::string()); if (type != cmake::MESSAGE) { - // we've overriden the message type, above, so force IssueMessage to use it - this->Makefile->IssueMessage(type, message, true); + // we've overriden the message type, above, so display it directly + cmake* cm = this->Makefile->GetCMakeInstance(); + cm->DisplayMessage(type, message, this->Makefile->GetBacktrace()); } else { if (status) { this->Makefile->DisplayStatus(message.c_str(), -1); diff --git a/Source/cmake.cxx b/Source/cmake.cxx index 657091b42..594eebf29 100644 --- a/Source/cmake.cxx +++ b/Source/cmake.cxx @@ -2294,16 +2294,14 @@ void displayMessage(cmake::MessageType t, std::ostringstream& msg) } void cmake::IssueMessage(cmake::MessageType t, std::string const& text, - cmListFileBacktrace const& backtrace, - bool force) const + cmListFileBacktrace const& backtrace) const { - if (!force) { - // override the message type, if needed, for warnings and errors - cmake::MessageType override = this->ConvertMessageType(t); - if (override != t) { - t = override; - force = true; - } + bool force = false; + // override the message type, if needed, for warnings and errors + cmake::MessageType override = this->ConvertMessageType(t); + if (override != t) { + t = override; + force = true; } if (!force && !this->IsMessageTypeVisible(t)) { diff --git a/Source/cmake.h b/Source/cmake.h index 1d63ede80..523c576d3 100644 --- a/Source/cmake.h +++ b/Source/cmake.h @@ -381,8 +381,7 @@ public: /** Display a message to the user. */ void IssueMessage( cmake::MessageType t, std::string const& text, - cmListFileBacktrace const& backtrace = cmListFileBacktrace(), - bool force = false) const; + cmListFileBacktrace const& backtrace = cmListFileBacktrace()) const; void DisplayMessage(cmake::MessageType t, std::string const& text, cmListFileBacktrace const& backtrace) const;