Simplified constructors, copy constructors & assignment operators

[eduar-hte]

what

Several constructors, copy constructors & assignment operators in the codebase can be simplified or even removed because they're not used.

why

Simplify the codebase making it easier to maintain.

NOTE: In the case of RuleMessage, it addresses one of the FIXME in the codebase, see here

misc

This is part of a series of PRs to improve performance of the library (5/n). Previous: #3231

[eduar-hte]

Failed conditions 8.8% Duplication on New Code (required ≤ 3%)

The tool is reporting duplicate code on changes to data the initialization of data members that were simplified (in src/transaction.cc).

The other two files mentioned are examples that use the same code to log rules (this PR only adjusts the existing lines to update how the data is retrieved). In library code (let's say, src dir) I'd obviously favor refactoring the code, but in the case of examples, it makes sense that they're independent to make it easier to use them as a simple reference and starting point for library users.

This PR is actually about removing code, but Sonarcloud will Sonarcloud...😊

[eduar-hte]

The tool is reporting duplicate code on changes to data the initialization of data members that were simplified (in src/transaction.cc).

I didn't look into updating the Transaction constructors in this PR, because these changes originated in those to simplify RuleMessage. These commits were part of another branch where use of RuleMessage is simplified, but I wanted to send them in a different PR to limit the scope of changes, as we've previously discussed.

PS: I'll take a look at the Transaction constructors and see if they can be simplified as well. They actually look identical, other than the id parameter, so they could be refactor to use a delegating constructor.

[eduar-hte]

PS: I'll take a look at the Transaction constructors and see if they can be simplified as well. They actually look identical, other than the id parameter, so they could be refactor to use a delegating constructor.

Done

PS: Notice that Sonarcloud once again run on stale code after the push, and is still showing the old version of transaction.cc and thus still flags duplicate code (in code that no longer exists). ¯_(ツ)_/¯

[eduar-hte]

Issues 17 New issues 0 Accepted issues

Edited commits to address the following issues reported by Sonarcloud:

• headers/modsecurity/rule_marker.h
  • Merge this "if" statement with the enclosing one
    • Existing code, not introduced in this PR.
• headers/modsecurity/rule_message.h
  • Do not use the constructor's initializer list for data member "m_severity". Use the in-class initializer instead (8)
    • Existing code, not introduced in this PR.
• headers/modsecurity/rule_unconditional.h
  • Add a using-declaration to this derived class to inherit the constructors of "RuleWithActions", and remove the ones you manually duplicated
• headers/modsecurity/rule_with_actions.h
  • Use "override" or "final" instead of "virtual"
• src/rule_with_actions.cc
  • This function should be declared "const"
    • Existing code, not introduced in this PR.
• src/rule_with_operator.cc
  • "std::move" should not be called on a const object.
    • Nice catch!
• src/transaction.cc
  • Make the type of this parameter a pointer-to-const. The current type of "id" is "char *"
    • Existing code, not introduced in this PR.

Did not address:

• src/transaction.cc
  • Replace this use of "void *" with a more meaningful type (2)
    • logCbData is a C-style opaque pointer required by the C callback interface.
  • Replace this const reference to "std::string" by a "std::string_view"
    • I wanted to address this, but using a std::string_view (which are passed by value) introduces a cppcheck warning, which would require a suppression.

[sonarcloud]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.5% Duplication on New Code

See analysis details on SonarCloud

[eduar-hte]

This is part of a series of PRs to improve performance of the library (5/n). Previous: #3231

Even though the goal of the PR is to simplify the codebase, commit 2ad87f6 (Reference RuleWithActions & Transaction object instead of copying values in RuleMessage) provides some performance improvements because it removes a number of variables copied from the associated rule & transaction.

These results were obtained running the benchmark test with 100'000 iterations with the OWASP CRS v4 and are presented as a reference:

• Mac mini (1st generation)
  • v3/master: 83.44 secs
  • This PR: 81.44 secs (2.4% improvement)
• Debian (bullseye) container running on WSL on Windows Dev Kit 2023
  • v3/master: 121.83 secs
  • This PR: 117.45 secs (3.59% improvement)

[airween]

Hi @eduar-hte

thanks again for this great PR!

Just one reminder to myself:

PS: I'll take a look at the Transaction constructors and see if they can be simplified as well. They actually look identical, other than the id parameter, so they could be refactor to use a delegating constructor.

Seems like the Transaction class public constructors are (not) a bit different that it documented in README.md:

Transaction *modsecTransaction = new Transaction(modsec, rules);

or in source documentation, so we need to align them.