Merge pull request #925 from hzeller/tigthen-clang-tidy1

Tighten clang tidy

.clang-tidy

@@ -2,12 +2,30 @@
 # Disabled some clang-analyzer static checks, need some more investigation.
 # TODO(hzeller) fix and re-enable clang-analyzer checks.
 # TODO(hzeller) Enable performance-* once somewhat silent output.
+# TODO(hzeller) Looking over magic numbers might be good, but probably
+#               not worthwhile hard-failing
+# TODO(hzeller) Fix qualified auto readability suggestions and enable.
+# TODO(hzeller) Explore anofalloff suggestions.
+# readability-make-member-function-const is great, but it also suggests that
+#    in cases where we return a non-const pointer. So good to check, not by
+#    default.
 Checks: >
   clang-diagnostic-*,clang-analyzer-*,
   -clang-analyzer-core.NonNullParamChecker,
   -clang-analyzer-cplusplus.NewDeleteLeaks,
   -clang-diagnostic-unused-const-variable,
   abseil-*,-abseil-no-namespace,
+  readability-*,
+  -readability-braces-around-statements,
+  -readability-named-parameter,
+  -readability-isolate-declaration,
+  -readability-redundant-access-specifiers,
+  -readability-implicit-bool-conversion,
+  -readability-magic-numbers,
+  -readability-else-after-return,
+  -readability-qualified-auto,
+  -readability-use-anyofallof,
+  -readability-make-member-function-const,
   google-*,
   -google-readability-braces-around-statements,
   -google-readability-todo,

common/analysis/file_analyzer.h

@@ -75,7 +75,7 @@ std::ostream& operator<<(std::ostream&, const RejectedToken&);
 class FileAnalyzer : public TextStructure {
  public:
   explicit FileAnalyzer(absl::string_view contents, absl::string_view filename)
-      : TextStructure(contents), filename_(filename), rejected_tokens_() {}
+      : TextStructure(contents), filename_(filename) {}
 
   virtual ~FileAnalyzer() {}
 

common/analysis/lint_rule_status.cc

@@ -34,7 +34,7 @@
 
 namespace verible {
 
-std::string AutoFix::Apply(const absl::string_view base) const {
+std::string AutoFix::Apply(absl::string_view base) const {
   std::string result;
   auto prev_start = base.cbegin();
   for (const auto& edit : edits) {

common/analysis/lint_rule_status.h

@@ -61,7 +61,7 @@ struct ReplacementEdit {
 // Collection of ReplacementEdits performing single violation fix.
 class AutoFix {
  public:
-  AutoFix() : description(), edits() {}
+  AutoFix() {}
   AutoFix(const AutoFix& other)
       : description(other.description), edits(other.edits) {}
   AutoFix(const AutoFix&& other)
@@ -83,7 +83,7 @@ class AutoFix {
   explicit AutoFix(ReplacementEdit edit) : AutoFix({edit}) {}
 
   // Applies the fix on a `base` and returns modified text.
-  std::string Apply(const absl::string_view base) const;
+  std::string Apply(absl::string_view base) const;
 
   bool AddEdits(const std::set<ReplacementEdit>& new_edits);
 
@@ -151,7 +151,7 @@ struct LintViolation {
 
 // LintRuleStatus represents the result of running a single lint rule.
 struct LintRuleStatus {
-  LintRuleStatus() : violations() {}
+  LintRuleStatus() {}
 
   LintRuleStatus(const std::set<LintViolation>& vs, absl::string_view rule_name,
                  const std::string& url)

common/analysis/syntax_tree_linter.h

@@ -49,7 +49,7 @@ namespace verible {
 //
 class SyntaxTreeLinter : public TreeContextVisitor {
  public:
-  SyntaxTreeLinter() : rules_() {}
+  SyntaxTreeLinter() {}
 
   void Visit(const SyntaxTreeLeaf& leaf) override;
   void Visit(const SyntaxTreeNode& node) override;

common/analysis/syntax_tree_search.cc

@@ -43,7 +43,7 @@ class SyntaxTreeSearcher : public TreeContextVisitor {
 
   void Search(const Symbol& root) { root.Accept(this); }
 
-  const std::vector<TreeSearchMatch> Matches() const { return matches_; }
+  const std::vector<TreeSearchMatch>& Matches() const { return matches_; }
 
  private:
   void CheckSymbol(const Symbol&);

common/formatting/align.cc

@@ -184,7 +184,7 @@ static void ColumnsTreeFormatter(
   static constexpr absl::string_view kCellSeparator = "|";
 
   struct Cell {
-    std::string text = "";
+    std::string text;
     char filler = ' ';
     std::size_t width = 0;
   };
@@ -301,7 +301,7 @@ struct AlignedColumnConfiguration {
   }
 };
 
-ColumnPositionTree* ColumnSchemaScanner::ReserveNewColumn(
+/* static */ ColumnPositionTree* ColumnSchemaScanner::ReserveNewColumn(
     ColumnPositionTree* parent_column, const Symbol& symbol,
     const AlignmentColumnProperties& properties, const SyntaxTreePath& path) {
   CHECK_NOTNULL(parent_column);

common/formatting/align.h

@@ -92,7 +92,7 @@ class ColumnSchemaScanner : public TreeContextPathVisitor {
   // 'path' represents relative position within the enclosing syntax subtree,
   // and is used as a key for ordering and matching columns.
   // Returns pointer to a created column or nullptr if column was not created.
-  ColumnPositionTree* ReserveNewColumn(
+  static ColumnPositionTree* ReserveNewColumn(
       ColumnPositionTree* parent_column, const Symbol& symbol,
       const AlignmentColumnProperties& properties, const SyntaxTreePath& path);
   ColumnPositionTree* ReserveNewColumn(
@@ -108,7 +108,7 @@ class ColumnSchemaScanner : public TreeContextPathVisitor {
   }
   // Reserve a subcolumn using subcolumn number appended to the parent's path
   // as the key.
-  ColumnPositionTree* ReserveNewColumn(
+  static ColumnPositionTree* ReserveNewColumn(
       ColumnPositionTree* parent_column, const Symbol& symbol,
       const AlignmentColumnProperties& properties) {
     CHECK_NOTNULL(parent_column);

common/formatting/unwrapped_line.cc

@@ -86,7 +86,7 @@ std::ostream& operator<<(std::ostream& stream, const UnwrappedLine& line) {
 }
 
 FormattedExcerpt::FormattedExcerpt(const UnwrappedLine& uwline)
-    : indentation_spaces_(uwline.IndentationSpaces()), tokens_() {
+    : indentation_spaces_(uwline.IndentationSpaces()) {
   tokens_.reserve(uwline.Size());
   // Convert working PreFormatTokens (computed from wrap optimization) into
   // decision-bound representation.

common/parser/parser_param.cc

@@ -33,8 +33,6 @@ ParserParam::ParserParam(TokenGenerator* token_stream)
     : token_stream_(token_stream),
       last_token_(TokenInfo::EOFToken()),
       root_(),
-      state_stack_(),
-      value_stack_(),
       max_used_stack_size_(0) {}
 
 ParserParam::~ParserParam() {}
@@ -52,7 +50,8 @@ void ParserParam::RecordSyntaxError(const SymbolPtr& symbol_ptr) {
 }
 
 template <typename T>
-static void move_stack(T** raw_stack, int64_t* size, std::vector<T>* stack) {
+static void move_stack(T** raw_stack, const int64_t* size,
+                       std::vector<T>* stack) {
   stack->resize(*size);
   std::move(*raw_stack, *raw_stack + *size, stack->begin());
 }

common/strings/obfuscator.h

@@ -46,7 +46,7 @@ class Obfuscator {
                        StringViewCompare>
       translator_type;
 
-  explicit Obfuscator(generator_type g) : generator_(g), translator_() {}
+  explicit Obfuscator(generator_type g) : generator_(g) {}
 
   // Declares a mapping from key-string to value-string that will be
   // used in obfuscation.  This is useful for applying previously used

common/text/concrete_syntax_leaf.h

@@ -66,9 +66,6 @@ class SyntaxTreeLeaf : public Symbol {
   TokenInfo token_;
 };
 
-// Returns a SyntaxTreeLeaf down_casted from a Symbol.
-const SyntaxTreeLeaf& SymbolCastToLeaf(const Symbol&);
-
 std::ostream& operator<<(std::ostream& os, const SyntaxTreeLeaf& l);
 
 }  // namespace verible

common/text/concrete_syntax_tree.h

@@ -73,7 +73,7 @@ struct ForwardChildren {
 // used by various language front-ends.
 class SyntaxTreeNode : public Symbol {
  public:
-  explicit SyntaxTreeNode(const int tag = kUntagged) : tag_(tag), children_() {}
+  explicit SyntaxTreeNode(const int tag = kUntagged) : tag_(tag) {}
 
   const std::vector<SymbolPtr>& children() const { return children_; }
   std::vector<SymbolPtr>& mutable_children() { return children_; }
@@ -114,10 +114,10 @@ class SyntaxTreeNode : public Symbol {
   }
 
   // Children accessor (mutable).
-  SymbolPtr& operator[](const size_t i);
+  SymbolPtr& operator[](size_t i);
 
   // Children accessor (const).
-  const SymbolPtr& operator[](const size_t i) const;
+  const SymbolPtr& operator[](size_t i) const;
 
   // Compares this node to an arbitrary symbol using the compare_tokens
   // function.

common/text/macro_definition.h

@@ -70,17 +70,13 @@ struct MacroCall {
   // Positional arguments to macro call.
   std::vector<DefaultTokenInfo> positional_arguments;
 
-  MacroCall() : has_parameters(false), positional_arguments() {}
+  MacroCall() : has_parameters(false) {}
 };
 
 class MacroDefinition {
  public:
   MacroDefinition(const TokenInfo& header, const TokenInfo& name)
-      : header_(header),
-        name_(name),
-        is_callable_(false),
-        parameter_info_array_(),
-        parameter_positions_() {}
+      : header_(header), name_(name), is_callable_(false) {}
 
   absl::string_view Name() const { return name_.text(); }
 

common/text/parser_verifier.h

@@ -42,17 +42,13 @@ namespace verible {
 class ParserVerifier : public TreeVisitorRecursive {
  public:
   ParserVerifier(const Symbol& root, const TokenStreamView& view)
-      : root_(root),
-        view_(view),
-        view_iterator_(view.begin()),
-        unmatched_tokens_() {}
+      : root_(root), view_(view), view_iterator_(view.begin()) {}
 
   ParserVerifier(const Symbol& root, const TokenStreamView& view,
                  const TokenComparator& token_comparator)
       : root_(root),
         view_(view),
         view_iterator_(view.begin()),
-        unmatched_tokens_(),
         token_comparator_(token_comparator) {}
 
   // Iterates through tree and stream view provided in constructor

common/text/tree_utils.cc

@@ -397,7 +397,7 @@ void PrettyPrinter::Visit(const SyntaxTreeLeaf& leaf) {
 }
 
 void RawSymbolPrinter::Visit(const SyntaxTreeNode& node) {
-  std::string tag_info = "";
+  std::string tag_info;
   const int tag = node.Tag().tag;
   if (tag != 0) tag_info = absl::StrCat("(tag: ", tag, ") ");
 

common/util/init_command_line.cc

@@ -45,8 +45,11 @@ static std::string GetBuildVersion() {
   return result;
 }
 
-std::vector<char*> InitCommandLine(absl::string_view usage, int* argc,
-                                   char*** argv) {
+// We might want to have argc edited in the future, hence non-const param.
+std::vector<char*> InitCommandLine(
+    absl::string_view usage,
+    int* argc,  // NOLINT(readability-non-const-parameter)
+    char*** argv) {
   absl::FlagsUsageConfig usage_config;
   usage_config.version_string = GetBuildVersion;
   absl::SetFlagsUsageConfig(usage_config);

verilog/CST/expression.cc

@@ -57,10 +57,9 @@ bool IsZero(const Symbol& expr) {
   if (child->Kind() != SymbolKind::kLeaf) return false;
   const auto& term = down_cast<const SyntaxTreeLeaf&>(*child);
   auto text = term.get().text();
-  if (text == "\'0") return true;
   // TODO(fangism): Could do more sophisticated constant expression evaluation
-  // but for now it is fine for this to conservatively return false.
-  return false;
+  // but for now this is a good first implementation.
+  return (text == "\'0");
 }
 
 bool ConstantIntegerValue(const verible::Symbol& expr, int* value) {

verilog/analysis/checkers/always_ff_non_blocking_rule.h

@@ -34,15 +34,15 @@ class AlwaysFFNonBlockingRule : public verible::SyntaxTreeLintRule {
 
   static const LintRuleDescriptor& GetDescriptor();
 
-  absl::Status Configure(const absl::string_view configuration) override;
+  absl::Status Configure(absl::string_view configuration) override;
   void HandleSymbol(const verible::Symbol& symbol,
                     const verible::SyntaxTreeContext& context) override;
 
   verible::LintRuleStatus Report() const override;
 
  private:
   // Detects entering and leaving relevant code inside always_ff
-  bool InsideBlock(const verible::Symbol& symbol, const int depth);
+  bool InsideBlock(const verible::Symbol& symbol, int depth);
   // Processes local declarations
   bool LocalDeclaration(const verible::Symbol& symbol);
 

verilog/analysis/checkers/forbidden_macro_rule.cc

@@ -89,8 +89,8 @@ verible::LintRuleStatus ForbiddenMacroRule::Report() const {
   return verible::LintRuleStatus(violations_, GetDescriptor());
 }
 
-std::string ForbiddenMacroRule::FormatReason(
-    const verible::SyntaxTreeLeaf& leaf) const {
+/* static */ std::string ForbiddenMacroRule::FormatReason(
+    const verible::SyntaxTreeLeaf& leaf) {
   const std::string function_name(leaf.get().text());
   const auto url = FindWithDefault(InvalidMacrosMap(), function_name, "");
   auto message = function_name + " is a forbidden macro";

verilog/analysis/checkers/forbidden_macro_rule.h

@@ -50,7 +50,7 @@ class ForbiddenMacroRule : public verible::SyntaxTreeLintRule {
   verible::LintRuleStatus Report() const override;
 
  private:
-  std::string FormatReason(const verible::SyntaxTreeLeaf& leaf) const;
+  static std::string FormatReason(const verible::SyntaxTreeLeaf& leaf);
 
   // Set of invalid macros, mapped to style guide links.
   static const std::map<std::string, std::string>& InvalidMacrosMap();

verilog/analysis/checkers/forbidden_symbol_rule.cc

@@ -95,8 +95,8 @@ verible::LintRuleStatus ForbiddenSystemTaskFunctionRule::Report() const {
   return verible::LintRuleStatus(violations_, GetDescriptor());
 }
 
-std::string ForbiddenSystemTaskFunctionRule::FormatReason(
-    const verible::SyntaxTreeLeaf& leaf) const {
+/* static */ std::string ForbiddenSystemTaskFunctionRule::FormatReason(
+    const verible::SyntaxTreeLeaf& leaf) {
   const auto function_name = std::string(leaf.get().text());
   const auto replacement =
       FindWithDefault(InvalidSymbolsMap(), function_name, "");

verilog/analysis/checkers/forbidden_symbol_rule.h

@@ -50,7 +50,7 @@ class ForbiddenSystemTaskFunctionRule : public verible::SyntaxTreeLintRule {
   verible::LintRuleStatus Report() const override;
 
  private:
-  std::string FormatReason(const verible::SyntaxTreeLeaf& leaf) const;
+  static std::string FormatReason(const verible::SyntaxTreeLeaf& leaf);
 
   // Set of invalid functions and suggested replacements
   static const std::map<std::string, std::string>& InvalidSymbolsMap();

verilog/analysis/checkers/module_instantiation_rules.cc

@@ -178,8 +178,8 @@ void ModulePortRule::HandleSymbol(const verible::Symbol& symbol,
 // contains all named ports
 // Returns false if node has 2 or more children and at least
 // one child is an unnamed positional port
-bool ModulePortRule::IsPortListCompliant(
-    const verible::SyntaxTreeNode& port_list_node) const {
+/* static */ bool ModulePortRule::IsPortListCompliant(
+    const verible::SyntaxTreeNode& port_list_node) {
   const auto& children = port_list_node.children();
   auto port_count = std::count_if(
       children.begin(), children.end(),

verilog/analysis/checkers/module_instantiation_rules.h

@@ -57,7 +57,8 @@ class ModulePortRule : public verible::SyntaxTreeLintRule {
  private:
   // Returns false if a port list node is in violation of this rule and
   // true if it is not.
-  bool IsPortListCompliant(const verible::SyntaxTreeNode& port_list_node) const;
+  static bool IsPortListCompliant(
+      const verible::SyntaxTreeNode& port_list_node);
 
   std::set<verible::LintViolation> violations_;
 };

verilog/analysis/checkers/parameter_name_style_rule.h

@@ -47,8 +47,8 @@ class ParameterNameStyleRule : public verible::SyntaxTreeLintRule {
 
  private:
   // Format diagnostic message.
-  std::string ViolationMsg(absl::string_view symbol_type,
-                           uint32_t allowed_bitmap);
+  static std::string ViolationMsg(absl::string_view symbol_type,
+                                  uint32_t allowed_bitmap);
 
   enum StyleChoicesBits {
     kUpperCamelCase = (1 << 0),

verilog/analysis/checkers/plusarg_assignment_rule.cc

@@ -74,7 +74,7 @@ verible::LintRuleStatus PlusargAssignmentRule::Report() const {
   return verible::LintRuleStatus(violations_, GetDescriptor());
 }
 
-std::string PlusargAssignmentRule::FormatReason() const {
+/* static */ std::string PlusargAssignmentRule::FormatReason() {
   return absl::StrCat("Do not use ", kForbiddenFunctionName,
                       " to access plusargs, use ", kCorrectFunctionName,
                       " instead.");

verilog/analysis/checkers/plusarg_assignment_rule.h

@@ -49,7 +49,7 @@ class PlusargAssignmentRule : public verible::SyntaxTreeLintRule {
   verible::LintRuleStatus Report() const override;
 
  private:
-  std::string FormatReason() const;
+  static std::string FormatReason();
   std::set<verible::LintViolation> violations_;
 };
 

verilog/analysis/checkers/port_name_suffix_rule.cc

@@ -85,8 +85,8 @@ void PortNameSuffixRule::Violation(absl::string_view direction,
   }
 }
 
-bool PortNameSuffixRule::IsSuffixCorrect(const absl::string_view suffix,
-                                         const absl::string_view direction) {
+bool PortNameSuffixRule::IsSuffixCorrect(absl::string_view suffix,
+                                         absl::string_view direction) {
   static const std::map<absl::string_view, std::set<absl::string_view>>
       suffixes = {{"input", {"i", "ni", "pi"}},
                   {"output", {"o", "no", "po"}},