fixed some clang-tidy warnings by firewave · Pull Request #3080 · cppcheck-opensource/cppcheck

firewave · 2021-01-24T11:21:19Z

No description provided.

danmar · 2021-01-24T14:20:58Z

 #include "common.h"

-static bool executeCommand(std::string exe, std::vector<std::string> args, std::string redirect, std::string *output)
+static bool executeCommand(const std::string& exe, const std::vector<std::string> &args, const std::string& redirect, std::string *output)


hmm.. shouldn't this use the exact same parameters as executeCommand in cppcheckexecutor.h?

What do you mean? the signature also already was different.

This is a callback used by CppCheck. The CppCheck constructor is:

CppCheck(ErrorLogger &errorLogger, bool useGlobalSuppressions, std::function<bool(std::string,std::vector<std::string>,std::string,std::string*)> executeCommand);

I believe executeCommand should match that.

the signature also already was different.

then that should be fixed imho.

You can update the callback signature if you want.

danmar · 2021-01-24T14:25:36Z

-        Token* start = const_cast<Token*>(it->bodyStart);
-        Token* end = const_cast<Token*>(it->bodyEnd);
-        if (it->type == Scope::eGlobal) {
+    for (Scope &it : scopeList) {


I would prefer the name scope. I guess you wanted to limit the change somewhat.

Will adjust.

pfultz2 · 2021-01-25T03:36:00Z

                             const Settings* settings)
 {
-    std::list<ValueFlow::Value> values = {val};
+    std::list<ValueFlow::Value> values = {std::move(val)};


If we move the value it will always copy because its a const ref.

The parameter is not a const ref - it's passed byvalue.

Clang or clang-tidy (can't remember) will also warn about such cases where a std::move() doesn't do anything.

I see its val2 thats passed by const ref.

But this is using val.

danmar · 2021-02-21T08:01:18Z

                    if (idxValues.empty() ||
                    std::any_of(idxValues.begin(), idxValues.end(), [&](const ValueFlow::Value& vidx) {
-                    if (vidx.isImpossible())
+                        if (vidx.isImpossible())


won't this be changed back by astyle? did you try that.

No longer an issue with uncrustify.

danmar

hm.. a split up PR would have been easier to review .. 99% of this is ok and could have been merged.

firewave · 2021-10-10T21:53:27Z

Note about the unfixed modernize-loop-convert warnings - these are cases where const_iterator is used. Unfortunately the for range loop is not using that but the regular iterator, so modernizing this actually changes the behavior. I think this can be worked around by wrapping the variable in question with std::as_const() but that is a C++17 feature. I want to file a ticket about that with clang-tidy at some point after I researched this a bit more - so probably never ... :/

firewave · 2021-10-13T11:58:28Z

@danmar This is ready for another review.

danmar · 2022-02-12T18:00:45Z

    /**
     * Execute a shell command and read the output from it. Returns true if command terminated successfully.
     */
-    static bool executeCommand(std::string exe, std::vector<std::string> args, const std::string &redirect, std::string *output_);


I think this should have the same signature as the executeCommand in gui/checkThread. It's used for the same callback. It's called from lib/cppcheck.cpp

I have no adjusted this yet.

danmar · 2022-02-12T18:04:11Z

    if (isC())
        return;

-    const Token * templateToken = nullptr;


No warning from Cppcheck about this variable? Do you know if there is a ticket?

when I look in the source code I do not see this? A rebase is needed?
https://github.com/danmar/cppcheck/blob/main/lib/tokenize.cpp#L4498

Yes, I didn't rebase this yet. Just commented on an open conversation.

I think this might not have been detected by cppcheck and requires a ticket. I think somebody else removed it.

firewave · 2022-05-02T06:28:23Z

I finally addressed all remarks. So this can be reviewed again. Sorry it took so long.

…ppcheck-opensource#3080 and adjusted callers to avoid unnecessary copies

danmar reviewed Jan 24, 2021

View reviewed changes

pfultz2 reviewed Jan 25, 2021

View reviewed changes

firewave force-pushed the tidy-22 branch from 3537ffb to f362def Compare January 28, 2021 17:37

danmar reviewed Feb 21, 2021

View reviewed changes

firewave marked this pull request as draft September 4, 2021 11:13

firewave force-pushed the tidy-22 branch 2 times, most recently from 5da0c84 to a3796b5 Compare October 10, 2021 21:49

firewave marked this pull request as ready for review October 13, 2021 11:58

danmar reviewed Feb 12, 2022

View reviewed changes

firewave marked this pull request as draft February 13, 2022 16:32

firewave added 4 commits May 2, 2022 08:21

enabled readability-misleading-indentation clang-tidy warnings

2818ce9

fixed some performance-unnecessary-value-param clang-tidy warnings

6f637fc

fixed some modernize-loop-convert clang-tidy warnings

d78e80f

.gitignore: added UpgradeLog*.htm

aca2f17

firewave force-pushed the tidy-22 branch from a3796b5 to aca2f17 Compare May 2, 2022 06:27

firewave marked this pull request as ready for review May 2, 2022 06:28

firewave changed the title ~~fixed some clang-tidy and compiler warnings~~ fixed some clang-tidy warnings May 3, 2022

danmar merged commit c710335 into cppcheck-opensource:main May 8, 2022

firewave deleted the tidy-22 branch May 8, 2022 18:57

firewave added a commit to firewave/cppcheck that referenced this pull request Sep 22, 2022

rolled back some performance-unnecessary-value-param changes from c…

6b44a05

…ppcheck-opensource#3080 and adjusted callers to avoid unnecessary copies

firewave added a commit to firewave/cppcheck that referenced this pull request Oct 8, 2022

rolled back some performance-unnecessary-value-param changes from c…

7c976de

…ppcheck-opensource#3080 and adjusted callers to avoid unnecessary copies

firewave added a commit to firewave/cppcheck that referenced this pull request Oct 8, 2022

rolled back some performance-unnecessary-value-param changes from c…

444eeb6

…ppcheck-opensource#3080 and adjusted callers to avoid unnecessary copies

firewave added a commit to firewave/cppcheck that referenced this pull request Dec 19, 2022

rolled back some performance-unnecessary-value-param changes from c…

2fa3275

…ppcheck-opensource#3080 and adjusted callers to avoid unnecessary copies

firewave added a commit to firewave/cppcheck that referenced this pull request Dec 19, 2022

rolled back some performance-unnecessary-value-param changes from c…

d0e3b10

…ppcheck-opensource#3080 and adjusted callers to avoid unnecessary copies

firewave added a commit to firewave/cppcheck that referenced this pull request Mar 2, 2023

rolled back some performance-unnecessary-value-param changes from c…

cf92432

…ppcheck-opensource#3080 and adjusted callers to avoid unnecessary copies

firewave added a commit to firewave/cppcheck that referenced this pull request Sep 1, 2023

rolled back some performance-unnecessary-value-param changes from c…

512dd37

…ppcheck-opensource#3080 and adjusted callers to avoid unnecessary copies

firewave added a commit to firewave/cppcheck that referenced this pull request Feb 5, 2024

rolled back some performance-unnecessary-value-param changes from c…

a868c31

…ppcheck-opensource#3080 and adjusted callers to avoid unnecessary copies

firewave added a commit to firewave/cppcheck that referenced this pull request Feb 5, 2024

rolled back some performance-unnecessary-value-param changes from c…

ac9eab5

…ppcheck-opensource#3080 and adjusted callers to avoid unnecessary copies

firewave added a commit to firewave/cppcheck that referenced this pull request Feb 5, 2024

rolled back some performance-unnecessary-value-param changes from c…

e1322ec

…ppcheck-opensource#3080 and adjusted callers to avoid unnecessary copies

firewave added a commit to firewave/cppcheck that referenced this pull request Feb 5, 2024

rolled back some performance-unnecessary-value-param changes from c…

4f1512a

…ppcheck-opensource#3080 and adjusted callers to avoid unnecessary copies

firewave added a commit to firewave/cppcheck that referenced this pull request Feb 5, 2024

rolled back some performance-unnecessary-value-param changes from c…

8d594e4

…ppcheck-opensource#3080 and adjusted callers to avoid unnecessary copies

Conversation

firewave commented Jan 24, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

firewave commented Oct 10, 2021

Uh oh!

firewave commented Oct 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

firewave commented May 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

danmar left a comment •

edited

Loading