NOTE: Please always add `static_assert` on datatype before using `FmtCore::format`

[kirk0830]

Describe the Code Quality Issue

After a very long time debugging, @jinzx10, @caic99 and I find the bug reported by issue #4540. See the following code piece: https://github.com/deepmodeling/abacus-develop/blob/db90f92ea2805026de20358b18df2f4c84d61c2c/source/module_cell/klist.cpp#L776 what will happen if: https://github.com/deepmodeling/abacus-develop/blob/db90f92ea2805026de20358b18df2f4c84d61c2c/source/module_cell/klist.cpp#L944-L953 ? It will trigger an undefined behavior because the double is parsed like int by formatter (the ABACUS in-built library for formatting strings, implemented in module_base/formatter.h), that cases recent failure in integrated test.

The implementation of function FmtCore::format is quite simple: https://github.com/deepmodeling/abacus-develop/blob/db90f92ea2805026de20358b18df2f4c84d61c2c/source/module_base/formatter.h#L40-L48 But the c-style function snprintf itself cannot identify the datatype. So I would suggest to use something like static_assert before using this function.

Additional Context

No response

Task list for Issue attackers (only for developers)

• [ ] Identify the specific code file or section with the code quality issue.
• [ ] Investigate the issue and determine the root cause.
• [ ] Research best practices and potential solutions for the identified issue.
• [ ] Refactor the code to improve code quality, following the suggested solution.
• [ ] Ensure the refactored code adheres to the project's coding standards.
• [ ] Test the refactored code to ensure it functions as expected.
• [ ] Update any relevant documentation, if necessary.
• [ ] Submit a pull request with the refactored code and a description of the changes made.

[kirk0830]

@WHUweiqingzhou FYI

[jinzx10]

Most compilers provide a "-Wformat" option for basic type checks for printf/sprintf/..., which would emit a warning message when there is a type mismatch. For example, compiling the following code

#include <cstdio>

int main() {
    double d = 3;
    std::snprintf(nullptr, 0, "%4i", d);
    return 0;
}

will get something like

hw.cpp: In function ‘int main()’:
hw.cpp:41:34: warning: format ‘%i’ expects argument of type ‘int’, but argument 4 has type ‘double’ [-Wformat=]
   41 |     std::snprintf(nullptr, 0, "%4i", d);
      |                                ~~^   ~
      |                                  |   |
      |                                  int double
      |                                %4f

I'm not sure if "-Wformat" is enabled by default [at least it is so on my everyday compiler gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04)], but even it is enabled, the real question is, "-Wformat" is merely a compile-time check that is unable to be carried out for argument types agains a format string passed via function argument, as it is in FmtCore. That is to say, "-Wformat" would fail to detect bugs like

#include <cstdio>

void foobar(const char* fmt, double i) {
    std::snprintf(nullptr, 0, fmt, i);
}

int main() {
    foobar("%4i", 3.0);
    return 0;
}

It might, however, be possible to let snprintf knows the format string during compilation, as long as the format string is a const expression (e.g., making "foobar" a function template and fmt a template parameter). Unfortunately, using string literals as template parameters is a c++20 feature; achieving this within c++11, if possible, might need some metaprogramming.

[caic99]

@jinzx10 IMHO using macro definition would be a good replacement

#include <cstdio>

#define foobar_safe(fmt, i) std::snprintf(nullptr, 0, fmt, i)

int main() {
    foobar_safe("%4i", 3.0);
}

This grammar generates warning correctly.

[kirk0830]

@caic99 I will use the following version:

#include <cstdio>

#define __FMTCORE_DATA_CHECK__(fmt, ...) std::snprintf(nullptr, 0, fmt, __VA_ARGS__)

int main() {
    const int right = 3;
    const double wrong = 3.0;

    __FMTCORE_DATA_CHECK__("%4d%4d", right, wrong);
}

[mohanchen]

I think in the beginning of the issue, there should be a link to explain what is 'formatter' @kirk0830

[kirk0830]

I think in the beginning of the issue, there should be a link to explain what is 'formatter' @kirk0830

Thanks, I have added a short explanation about formatter in issue descrption

[WHUweiqingzhou]

I move this issue to discussion Note for Developer.