Skip to content

cleaned up handling of specifying multiple libraries at once #6593

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
chrchr-github merged 4 commits into from
Jul 16, 2024
Merged

cleaned up handling of specifying multiple libraries at once #6593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8c120ed
test loading of multiple libraries at once
firewave Jul 12, 2024
e778473
added `splitString()` to `utils.{cpp|h}`
firewave Jul 12, 2024
48704ee
split list of libraries early in CLI
firewave Jul 12, 2024
0ce4f3b
bail out early on empty library in CLI
firewave Jul 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion cli/cmdlineparser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -870,7 +870,14 @@ CmdLineParser::Result CmdLineParser::parseFromArgs(int argc, const char* const a

// --library
else if (std::strncmp(argv[i], "--library=", 10) == 0) {
mSettings.libraries.emplace_back(argv[i] + 10);
std::list<std::string> libs = splitString(argv[i] + 10, ',');
for (auto& l : libs) {
if (l.empty()) {
mLogger.printError("empty library specified.");
return Result::Fail;
}
mSettings.libraries.emplace_back(std::move(l));
}
}

// Set maximum number of #ifdef configurations to check
Expand Down
17 changes: 1 addition & 16 deletions lib/library.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,23 +184,8 @@ static void gettokenlistfromvalid(const std::string& valid, bool cpp, TokenList&

Library::Error Library::load(const char exename[], const char path[], bool debug)
{
// TODO: remove handling of multiple libraries at once?
if (std::strchr(path,',') != nullptr) {
if (debug)
std::cout << "handling multiple libraries '" + std::string(path) + "'" << std::endl;
std::string p(path);
for (;;) {
const std::string::size_type pos = p.find(',');
if (pos == std::string::npos)
break;
const Error &e = load(exename, p.substr(0,pos).c_str());
if (e.errorcode != ErrorCode::OK)
return e;
p = p.substr(pos+1);
}
if (!p.empty())
return load(exename, p.c_str());
return Error();
throw std::runtime_error("handling of multiple libraries not supported");
}

const bool is_abs_path = Path::isAbsolute(path);
Expand Down
19 changes: 19 additions & 0 deletions lib/utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

#include <algorithm>
#include <cctype>
#include <cstring>
#include <iterator>
#include <stack>
#include <utility>
Expand Down Expand Up @@ -184,3 +185,21 @@ std::string replaceEscapeSequences(const std::string &source) {
return result;
}


std::list<std::string> splitString(const std::string& str, char sep)
Copy link
Copy Markdown
Collaborator

@chrchr-github chrchr-github Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's bikeshed about this unimportant utility function, which seems quite inefficient.

We could use a very short regex version (which also doesn't produce empty strings), or this replacement:

std::list<std::string> splitString2(const std::string& str, char sep)
{
    std::list<std::string> l;
    std::string::size_type pos = 0;
    for (;;) {
        const std::string::size_type newPos = str.find(sep, pos);
        l.push_back(str.substr(pos, newPos - pos));
        if (newPos == std::string::npos) {
            break;
        }
        pos = newPos + 1;
    }
    return l;
}

https://godbolt.org/z/85E8xGThd

Copy link
Copy Markdown
Collaborator Author

@firewave firewave Jul 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already a simpler version locally (which might be identical to your version) which I will add in a follow-up PR which will also extend its usage. I wanted to keep this topical and not introduce any functional non-refactoring changes.

Copy link
Copy Markdown
Collaborator

@chrchr-github chrchr-github Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.
There still is a merge conflict.

{
if (std::strchr(str.c_str(), sep) == nullptr)
return {str};

std::list<std::string> l;
std::string p(str);
for (;;) {
const std::string::size_type pos = p.find(sep);
if (pos == std::string::npos)
break;
l.push_back(p.substr(0,pos));
p = p.substr(pos+1);
}
l.push_back(std::move(p));
return l;
}
9 changes: 9 additions & 0 deletions lib/utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include <functional>
#include <initializer_list>
#include <limits>
#include <list>
#include <stdexcept>
#include <string>
#include <type_traits>
Expand Down Expand Up @@ -390,4 +391,12 @@ static inline T* empty_if_null(T* p)
return p ? p : "";
}

/**
* Split string by given sperator.
* @param str The string to split
* @param sep The seperator
* @return The list of seperate strings (including empty ones). The whole input string if no seperator found.
*/
CPPCHECKLIB std::list<std::string> splitString(const std::string& str, char sep);

#endif
24 changes: 24 additions & 0 deletions test/cli/other_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1728,3 +1728,27 @@ def test_lib_lookup_nofile(tmpdir):
"looking for library '{}/cfg/gtk.cfg'".format(exepath),
'Checking {} ...'.format(test_file)
]


def test_lib_lookup_multi(tmpdir):
test_file = os.path.join(tmpdir, 'test.c')
with open(test_file, 'wt'):
pass

exitcode, stdout, _, exe = cppcheck_ex(['--library=posix,gnu', '--debug-lookup', test_file])
exepath = os.path.dirname(exe)
if sys.platform == 'win32':
exepath = exepath.replace('\\', '/')
assert exitcode == 0, stdout
lines = __remove_std_lookup_log(stdout.splitlines(), exepath)
assert lines == [
"looking for library 'posix'",
"looking for library 'posix.cfg'",
"looking for library '{}/posix.cfg'".format(exepath),
"looking for library '{}/cfg/posix.cfg'".format(exepath),
"looking for library 'gnu'",
"looking for library 'gnu.cfg'",
"looking for library '{}/gnu.cfg'".format(exepath),
"looking for library '{}/cfg/gnu.cfg'".format(exepath),
'Checking {} ...'.format(test_file)
]
27 changes: 27 additions & 0 deletions test/testcmdlineparser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,9 @@ class TestCmdlineParser : public TestFixture {
TEST_CASE(signedCharUnsignedChar);
TEST_CASE(library);
TEST_CASE(libraryMissing);
TEST_CASE(libraryMultiple);
TEST_CASE(libraryMultipleEmpty);
TEST_CASE(libraryMultipleEmpty2);
TEST_CASE(suppressXml);
TEST_CASE(suppressXmlEmpty);
TEST_CASE(suppressXmlMissing);
Expand Down Expand Up @@ -2447,6 +2450,30 @@ class TestCmdlineParser : public TestFixture {
ASSERT_EQUALS("cppcheck: Failed to load library configuration file 'posix2'. File not found\n", logger->str());
}

void libraryMultiple() {
REDIRECT;
const char * const argv[] = {"cppcheck", "--library=posix,gnu", "file.cpp"};
ASSERT_EQUALS_ENUM(CmdLineParser::Result::Success, parser->parseFromArgs(3, argv));
ASSERT_EQUALS(2, settings->libraries.size());
auto it = settings->libraries.cbegin();
ASSERT_EQUALS("posix", *it++);
ASSERT_EQUALS("gnu", *it);
}

void libraryMultipleEmpty() {
REDIRECT;
const char * const argv[] = {"cppcheck", "--library=posix,,gnu", "file.cpp"};
ASSERT_EQUALS(false, parser->fillSettingsFromArgs(3, argv));
ASSERT_EQUALS("cppcheck: error: empty library specified.\n", logger->str());
}

void libraryMultipleEmpty2() {
REDIRECT;
const char * const argv[] = {"cppcheck", "--library=posix,gnu,", "file.cpp"};
ASSERT_EQUALS(false, parser->fillSettingsFromArgs(3, argv));
ASSERT_EQUALS("cppcheck: error: empty library specified.\n", logger->str());
}

void suppressXml() {
REDIRECT;
ScopedFile file("suppress.xml",
Expand Down
45 changes: 45 additions & 0 deletions test/testutils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ class TestUtils : public TestFixture {
TEST_CASE(trim);
TEST_CASE(findAndReplace);
TEST_CASE(replaceEscapeSequences);
TEST_CASE(splitString);
}

void isValidGlobPattern() const {
Expand Down Expand Up @@ -438,6 +439,50 @@ class TestUtils : public TestFixture {
ASSERT_EQUALS("\\", ::replaceEscapeSequences("\\\\"));
ASSERT_EQUALS("\"", ::replaceEscapeSequences("\\\""));
}

void splitString() const {
{
const auto l = ::splitString("test", ',');
ASSERT_EQUALS(1, l.size());
ASSERT_EQUALS("test", *l.cbegin());
}
{
const auto l = ::splitString("test,test", ';');
ASSERT_EQUALS(1, l.size());
ASSERT_EQUALS("test,test", *l.cbegin());
}
{
const auto l = ::splitString("test,test", ',');
ASSERT_EQUALS(2, l.size());
auto it = l.cbegin();
ASSERT_EQUALS("test", *it++);
ASSERT_EQUALS("test", *it);
}
{
const auto l = ::splitString("test,test,", ',');
ASSERT_EQUALS(3, l.size());
auto it = l.cbegin();
ASSERT_EQUALS("test", *it++);
ASSERT_EQUALS("test", *it++);
ASSERT_EQUALS("", *it);
}
{
const auto l = ::splitString("test,,test", ',');
ASSERT_EQUALS(3, l.size());
auto it = l.cbegin();
ASSERT_EQUALS("test", *it++);
ASSERT_EQUALS("", *it++);
ASSERT_EQUALS("test", *it);
}
{
const auto l = ::splitString(",test,test", ',');
ASSERT_EQUALS(3, l.size());
auto it = l.cbegin();
ASSERT_EQUALS("", *it++);
ASSERT_EQUALS("test", *it++);
ASSERT_EQUALS("test", *it);
}
}
};

REGISTER_TEST(TestUtils)