[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[lmi] Enabling '-Weffc++' by default
From: |
Greg Chicares |
Subject: |
[lmi] Enabling '-Weffc++' by default |
Date: |
Tue, 9 Jun 2020 22:34:34 +0000 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 |
I had always thought that building with '-Weffc++' was sure to produce
reams of output, mostly pointing to issues in third-party libraries
beyond my control. However, apparently thanks again to '-isystem', it
seems almost within our grasp for lmi, and wouldn't that be glorious?
Specifically, if I do this in 'workhorse.make':
gcc_common_extra_warnings := \
-Wcast-qual \
+ -Weffc++ \
and then
make clobber && make install
I still get reams of output, but almost all of it just says:
'foo' should be initialized in the member initialization list
and it seems reasonable to suppose that all 5157 of them can and
should be fixed. Then we'll have only the four '-Weffc++' issues
listed below:
/opt/lmi/src/lmi/dbdict.hpp:37:14: error: base class ‘class
cache_file_reads<DBDictionary>’ has accessible non-virtual destructor
[-Werror=non-virtual-dtor]
/opt/lmi/src/lmi/html.hpp:137:7: error: ‘class html::attribute’ has pointer
data members [-Werror=effc++]
/opt/lmi/src/lmi/html.hpp:137:7: error: but does not override
‘html::attribute(const html::attribute&)’ [-Werror=effc++]
/opt/lmi/src/lmi/html.hpp:137:7: error: or ‘operator=(const
html::attribute&)’ [-Werror=effc++]
/opt/lmi/src/lmi/html.hpp:166:14: error: ‘class html::detail::any_element’ has
pointer data members [-Werror=effc++]
/opt/lmi/src/lmi/html.hpp:166:14: error: but does not override
‘html::detail::any_element(const html::detail::any_element&)’ [-Werror=effc++]
/opt/lmi/src/lmi/html.hpp:166:14: error: or ‘operator=(const
html::detail::any_element&)’ [-Werror=effc++]
/opt/lmi/src/lmi/pdf_command_wx.cpp:1321:7: error: ‘class
{anonymous}::standard_page’ has pointer data members [-Werror=effc++]
/opt/lmi/src/lmi/pdf_command_wx.cpp:1321:7: error: but does not override
‘{anonymous}::standard_page(const {anonymous}::standard_page&)’ [-Werror=effc++]
/opt/lmi/src/lmi/pdf_command_wx.cpp:1321:7: error: or ‘operator=(const
{anonymous}::standard_page&)’ [-Werror=effc++]
The first one (dbdict.hpp:37) is weird: it appears only with
'-Weffc++', but is flagged "[-Werror=non-virtual-dtor]", so
perhaps '-Weffc++' strengthens '-Wnon-virtual-dtor' checks?
Anyway, the class consists of just one typedef and one static
member function, so AFAICS is shouldn't even have a (nonempty)
vtable, and the warning is overly aggressive. But adding
+ virtual ~cache_file_reads() = default;
shuts it up, and seems harmless.
The last one (pdf_command_wx.cpp:1321) seems easy to fix by
doing away with the pointer data member:
- char const* const page_template_name_;
+ std::string const page_template_name_;
std::unique_ptr<wxHtmlContainerCell> page_body_cell_;
std::unique_ptr<wxHtmlContainerCell> header_cell_;
and I think that's correct. Vadim, was there some special
reason for using a C string in this particular case?
And would you please share your thoughts on the other two,
(html.hpp:137) and (html.hpp:166)? I hesitate to replace
"char const*" with string here, because I think you preferred
C strings for efficiency--and, unlike the (pdf_command_wx.cpp:1321)
case above, superficial changes wouldn't suffice here. Here is a
speculative patch for all four issues, and it treats the middle
two brutally, although the other changes might be okay:
----8<----8<----8<----8<----
diff --git a/cache_file_reads.hpp b/cache_file_reads.hpp
index 64b0cfe8..59322912 100644
--- a/cache_file_reads.hpp
+++ b/cache_file_reads.hpp
@@ -101,11 +101,11 @@ class file_cache
struct record
{
- retrieved_type data;
- std::time_t write_time;
+ retrieved_type data {};
+ std::time_t write_time {};
};
- std::map<std::string,record> cache_;
+ std::map<std::string,record> cache_ {};
};
} // namespace detail
@@ -128,6 +128,8 @@ class cache_file_reads
{
return detail::file_cache<T>::instance().retrieve_or_reload(filename);
}
+
+ virtual ~cache_file_reads() = default;
};
#endif // cache_file_reads_hpp
diff --git a/html.cpp b/html.cpp
index 41b01fa4..ce885ad4 100644
--- a/html.cpp
+++ b/html.cpp
@@ -58,7 +58,7 @@ extern element const tr ("tr");
std::string attribute::as_string() const
{
- std::string s(name_);
+ std::string s(name_.c_str());
if(!value_.empty())
{
s += "=";
@@ -75,8 +75,8 @@ std::string any_element::get_start() const
{
std::string s("<");
// Extra +1 for the space before attributes, even if it's not needed.
- s.reserve(1 + std::strlen(name_) + 1 + attributes_.length() + 1);
- s += name_;
+ s.reserve(1 + std::strlen(name_.c_str()) + 1 + attributes_.length() + 1);
+ s += name_.c_str();
if(!attributes_.empty())
{
s += " ";
@@ -116,10 +116,10 @@ void element::update_contents(std::string&& contents)
element::operator text() const
{
std::string s(get_start());
- s.reserve(s.length() + contents_.length() + 2 + std::strlen(name_) + 1);
+ s.reserve(s.length() + contents_.length() + 2 + std::strlen(name_.c_str())
+ 1);
s += contents_;
s += "</";
- s += name_;
+ s += name_.c_str();
s += ">";
return text::from_html(std::move(s));
diff --git a/html.hpp b/html.hpp
index 2e561fd3..bf4acb18 100644
--- a/html.hpp
+++ b/html.hpp
@@ -150,13 +150,13 @@ class attribute
std::string as_string() const;
private:
- attribute(char const* name, std::string&& value)
+ attribute(std::string const& name, std::string&& value)
:name_ {name}
,value_ {std::move(value)}
{
}
- char const* const name_;
+ std::string const name_;
std::string const value_;
};
@@ -179,7 +179,7 @@ class LMI_SO any_element
// Add the given attribute to our attributes string.
void update_attributes(attribute const& attr);
- char const* const name_;
+ std::string const name_;
private:
std::string attributes_;
diff --git a/input_sequence_parser.cpp b/input_sequence_parser.cpp
index 9100f2c5..91b68129 100644
--- a/input_sequence_parser.cpp
+++ b/input_sequence_parser.cpp
@@ -42,6 +42,7 @@ SequenceParser::SequenceParser
,bool a_keywords_only
)
:input_stream_ {input_expression}
+ ,diagnostics_ {}
,years_to_maturity_ {a_years_to_maturity}
,issue_age_ {a_issue_age}
,retirement_age_ {a_retirement_age}
diff --git a/input_sequence_parser.hpp b/input_sequence_parser.hpp
index b9086b4d..95712bf7 100644
--- a/input_sequence_parser.hpp
+++ b/input_sequence_parser.hpp
@@ -93,8 +93,8 @@ class SequenceParser final
void mark_diagnostic_context();
// Parser products.
- std::string diagnostic_messages_;
- std::vector<ValueInterval> intervals_;
+ std::string diagnostic_messages_ {};
+ std::vector<ValueInterval> intervals_ {};
// Streams for parser input and diagnostic messages.
std::istringstream input_stream_;
diff --git a/pdf_command_wx.cpp b/pdf_command_wx.cpp
index 83be5241..117a1da3 100644
--- a/pdf_command_wx.cpp
+++ b/pdf_command_wx.cpp
@@ -1402,7 +1402,7 @@ class standard_page : public numbered_page
if(!page_body_cell_)
{
throw std::runtime_error
- ("failed to parse template '" +
std::string{page_template_name_} + "'"
+ ("failed to parse template '" + page_template_name_ + "'"
);
}
@@ -1435,7 +1435,7 @@ class standard_page : public numbered_page
return header_cell_.get();
}
- char const* const page_template_name_;
+ std::string const page_template_name_;
std::unique_ptr<wxHtmlContainerCell> page_body_cell_;
std::unique_ptr<wxHtmlContainerCell> header_cell_;
std::vector<int> page_break_positions_;
diff --git a/workhorse.make b/workhorse.make
index 8c3a276e..8543dcca 100644
--- a/workhorse.make
+++ b/workhorse.make
@@ -541,6 +541,7 @@ postponed_gcc_cxx_warnings := \
gcc_common_extra_warnings := \
-Wcast-qual \
+ -Weffc++ \
bourn_cast_test.o: gcc_common_extra_warnings += \
-Wno-double-promotion \
---->8---->8---->8---->8----
- [lmi] Enabling '-Weffc++' by default,
Greg Chicares <=