CppCheck warnings - Progress summary and remaining items

[KaganCanSit]

Hello,

Based on the CppCheck report output from the PR request here, I made some edits to help as much as I could. Thank you very much for the quick response. Here is a list of what I did: #3509 - #3513 - #3514 - #3515 - #3516

For now, I have left the following groups as they are, as I have not reviewed the false-positive cases and have ignored the tests directory and the fmt subdirectory. However, I am not entirely sure about the changes I will make for these. That is why I wanted to get your thoughts first.

Working on different codebases and spending time on static analysis often helps me catch new details. I want to help. However, working on a different and unfamiliar codebase also significantly increases my risk of making mistakes.

spdlog is an ongoing project that helps many users. That's why I'm a bit hesitant. Thank you for bringing this project to us, for your effort, and for the time you've invested.

The remaining groups at this moment;

Function Static
-------------------------------------
include/spdlog/details/null_mutex.h:13:performance:functionStatic -> Technically the member function 'spdlog::details::null_mutex::lock' can be static (but you may consider moving to unnamed namespace).
include/spdlog/details/null_mutex.h:14:performance:functionStatic -> Technically the member function 'spdlog::details::null_mutex::unlock' can be static (but you may consider moving to unnamed namespace).
include/spdlog/sinks/rotating_file_sink.h:51:performance:functionStatic -> Technically the member function 'rotating_file_sink < std :: mutex >::rename_file_' can be static (but you may consider moving to unnamed namespace).
include/spdlog/sinks/rotating_file_sink.h:51:performance:functionStatic -> Technically the member function 'rotating_file_sink < spdlog :: details :: null_mutex >::rename_file_' can be static (but you may consider moving to unnamed namespace).
include/spdlog/sinks/daily_file_sink.h:146:performance:functionStatic -> Technically the member function 'daily_file_sink < std :: mutex , spdlog :: sinks :: daily_filename_calculator >::now_tm' can be static (but you may consider moving to unnamed namespace).
include/spdlog/sinks/daily_file_sink.h:146:performance:functionStatic -> Technically the member function 'daily_file_sink < std :: mutex , custom_daily_file_name_calculator >::now_tm' can be static (but you may consider moving to unnamed namespace).
include/spdlog/sinks/daily_file_sink.h:146:performance:functionStatic -> Technically the member function 'daily_file_sink < details :: null_mutex , daily_filename_calculator >::now_tm' can be static (but you may consider moving to unnamed namespace).
include/spdlog/sinks/daily_file_sink.h:146:performance:functionStatic -> Technically the member function 'spdlog::sinks::daily_file_sink::now_tm' can be static (but you may consider moving to unnamed namespace).


Passed By Value
-------------------------------------
include/spdlog/logger.h:79:performance:passedByValue -> Function parameter 'loc' should be passed by const reference.
include/spdlog/logger.h:102:performance:passedByValue -> Function parameter 'loc' should be passed by const reference.
include/spdlog/logger.h:115:performance:passedByValue -> Function parameter 'loc' should be passed by const reference.
include/spdlog/spdlog.h:149:performance:passedByValue -> Function parameter 'source' should be passed by const reference.
include/spdlog/spdlog.h:192:performance:passedByValue -> Function parameter 'source' should be passed by const reference.
include/spdlog/spdlog.h:149:performance:passedByValueCallback -> Function parameter 'source' should be passed by const reference. However it seems that 'log' is a callback function.
include/spdlog/sinks/udp_sink.h:39:performance:passedByValue -> Function parameter 'sink_config' should be passed by const reference.
include/spdlog/sinks/dist_sink.h:26:performance:passedByValue -> Function parameter 'sinks' should be passed by const reference.


Function Arguments Names Different
-------------------------------------
include/spdlog/details/log_msg-inl.h:38:style:funcArgNamesDifferent -> Function 'log_msg' argument 1 names different: declaration 'logger_name' definition 'a_logger_name'.
include/spdlog/spdlog-inl.h:42:style:funcArgNamesDifferent -> Function 'should_log' argument 1 names different: declaration 'lvl' definition 'log_level'.
include/spdlog/async_logger-inl.h:60:style:funcArgNamesDifferent -> Function 'backend_sink_it_' argument 1 names different: declaration 'incoming_log_msg' definition 'msg'.
include/spdlog/cfg/helpers-inl.h:73:style:funcArgNamesDifferent -> Function 'load_levels' argument 1 names different: declaration 'txt' definition 'input'.

---

Additionally, as I highlighted in #3515, I omitted the following warnings due to their broad impact.

It was ignored due to its large-scale impact:

include/spdlog/sinks/rotating_file_sink.h:33:style:functionConst -> Technically the member function 'rotating_file_sink < std :: mutex >::get_max_size' can be const.
include/spdlog/sinks/rotating_file_sink.h:35:style:functionConst -> Technically the member function 'rotating_file_sink < std :: mutex >::get_max_files' can be const.
include/spdlog/sinks/rotating_file_sink.h:33:style:functionConst -> Technically the member function 'rotating_file_sink < spdlog :: details :: null_mutex >::get_max_size' can be const.
include/spdlog/sinks/rotating_file_sink.h:35:style:functionConst -> Technically the member function 'rotating_file_sink < spdlog :: details :: null_mutex >::get_max_files' can be const.
include/spdlog/sinks/daily_file_sink.h:100:style:functionConst -> Technically the member function 'spdlog::sinks::daily_file_sink::filename' can be const.

These functions use std::lock_guard<Mutex> for thread-safety:

template <typename Mutex>
SPDLOG_INLINE std::size_t rotating_file_sink<Mutex>::get_max_size() {
    std::lock_guard<Mutex> lock(base_sink<Mutex>::mutex_);
    return max_size_;
}

However, the remaining warnings require the std::mutex object used from base_sink to be declared as mutable. Since this would affect all parts using base_sink, some of the warnings have been ignored.

Best regards.

[tt4g]

IMO: Function Static

Since null_mutex is intended to be used as a template parameter instead of std::mutex, it should not be declared static.

For rotating_file_sink and daily_file_sink, declaring them as static may break ABI compatibility.
Furthermore, if a feature enhancement requiring access to this becomes necessary after the functions are made static, it would cause problems.
Considering these points, they should not be declared as static.

[tt4g]

IMO: Passed By Value

The source code location objects targeted by warnings in logger.h and spdlog.h can optionally accept std::source_location.
The C++ specification does not guarantee that copy constructors and move constructors behave identically, so behavior may vary depending on the implementation: https://en.cppreference.com/w/cpp/utility/source_location/source_location.html

Other than the above, I believe they can be const &.

[tt4g]

Regarding Function Arguments Names Different, I believe the names should be corrected as the warning indicates.
This correction should (almost) never cause any issues.

[KaganCanSit]

Thank you for all the information and feedback @tt4g.

We share similar thoughts regarding the Function Static warnings. In this case, I will review the remaining Passed By Value and Function Arguments Names Different warnings.

The likely first and quick target will be Function Arguments Names Different. I haven't fully reviewed it yet, but naming the member variable contents in the struct with the m_ prefix seems much more reliable. Because when I checked with blame, I saw that there were compilation errors due to name resolution before, and the a_ prefix was used for the arguments taken as function parameters.

This is certainly a good approach, but considering the possibility of expansion, it seems healthier to organize the class's internal members rather than the external parameters (arguments). As I said, I will look into it; I'm not sure yet.

I'll create PR content as the process progresses. I'll close this discussion after a certain period of time.

Regards.

Update: I took a quick look at this. If we want to effectively address shadowing and warnings so they don't occur again, implementing the above solution seems like a good idea. However, changing the member names in the struct definition would break the API and require changes in too many files. Adding the a_ prefix to all of them doesn't seem logical either. So, I'm not sure at this point.

[tt4g]

Unifying the naming convention for member variables might be acceptable, but it seems like the scope of changes would become too extensive.
Since it is likely intentional that member variables for simple data types like log_msg do not have the m_ prefix, we should carefully consider whether to apply this change to all member variables.

We must also note that some users define custom sinks, so modifying member variables would necessitate corrections to their code as well.

[KaganCanSit]

Unifying the naming convention for member variables might be acceptable, but it seems like the scope of changes would become too extensive. Since it is likely intentional that member variables for simple data types like log_msg do not have the m_ prefix, we should carefully consider whether to apply this change to all member variables.

We must also note that some users define custom sinks, so modifying member variables would necessitate corrections to their code as well.

We are definitely on the same page, I agree. For this reason, I have disregarded some of the warnings, taking your feedback into account.

I have now completed the adjustments for the warnings here with #3519 and #3520.

I will keep this discussion open for a little longer (maximum 1 week) in case I made any mistakes or if you have any additional ideas, and then I will close it.

Thank you for sharing your knowledge; it was fun working on a different project. If you think I can help with anything, feel free to ping me.

Best regards.

[KaganCanSit]

The warnings mentioned in this discussion have been corrected with PR requests, except for those deemed unnecessary and those evaluated as false positives. For this reason, the discussion is closed with this message.

If there are any issues or necessary additional adjustments, you can contact me as I may have made mistakes.

Best regards.