Skip to content

Conversation

@Falital
Copy link
Contributor

@Falital Falital commented Nov 11, 2020

This is done in order to support files with the same in diffrent
directories. Added simple cli test for --plist-output cmd.
Added fix for #8289 an error is generated if the --plist-output
folder does not exist.

This is done in order to support files with the same in diffrent
directories. Added simple cli test for --plist-output cmd.
Added fix for #8289 an error is generated if the --plist-output
folder does not exist.
@Falital
Copy link
Contributor Author

Falital commented Nov 12, 2020

Hello i hope i peformed the pull request correctly. We are using https://github.com/Ericsson/codechecker for our static analyses tool that also includes cppcheck. This works really fine up to the point where a project contains the same file name in several directories. To work around this we added a hash at the end of the generated plist files. Thanks for the good work.

@danmar
Copy link
Owner

danmar commented Nov 12, 2020

@Falital thanks for the info.. good to know that this plist output is actually used

return true;
}

static bool folderExist(const std::string &path)
Copy link
Owner

Choose a reason for hiding this comment

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

I think you can move this to lib/path.cpp .. add a function Path::folderExist instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved function to Path module

else if (!endsWith(mSettings->plistOutput,'/'))
mSettings->plistOutput += '/';

printf("mSettings->plistOutput %s\n", mSettings->plistOutput.c_str());
Copy link
Owner

Choose a reason for hiding this comment

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

This printf is just some debug command I assume that you should remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes debug output i removed it.

This is done in order to support files with the same in diffrent
directories. Added simple cli test for --plist-output cmd.
Added fix for #8289 an error is generated if the --plist-output
folder does not exist.
This is done in order to support files with the same in diffrent
directories. Added simple cli test for --plist-output cmd.
Added fix for #8289 an error is generated if the --plist-output
folder does not exist.
Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

nits

lib/path.h Outdated
* @param path Path to be checked if it is a folder
* @return true if given path is a folder
*/
static bool folderExist(const std::string &path);
Copy link
Owner

Choose a reason for hiding this comment

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

there is a fileExists at the bottom so I think it's logical to put this above/below that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found similar functions in FileLister and moved them to Path adapted test etc.

else if (!endsWith(mSettings->plistOutput,'/'))
mSettings->plistOutput += '/';

if ((mSettings->plistOutput != "./") && (!Path::folderExist(mSettings->plistOutput)))
Copy link
Owner

Choose a reason for hiding this comment

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

The (mSettings->plistOutput != "./") && seems redundant to me.

Copy link
Contributor Author

@Falital Falital Nov 12, 2020

Choose a reason for hiding this comment

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

Removed redundant check and also converted path to native so this can also work in windows.

This is done in order to support files with the same in diffrent
directories. Added simple cli test for --plist-output cmd.
Added fix for #8289 an error is generated if the --plist-output
folder does not exist.
This is done in order to support files with the same in diffrent
directories. Added simple cli test for --plist-output cmd.
Added fix for #8289 an error is generated if the --plist-output
folder does not exist.
This is done in order to support files with the same in diffrent
directories. Added simple cli test for --plist-output cmd.
Added fix for #8289 an error is generated if the --plist-output
folder does not exist.
This is done in order to support files with the same in diffrent
directories. Added simple cli test for --plist-output cmd.
Added fix for #8289 an error is generated if the --plist-output
folder does not exist.
path = Path::fromNativeSeparators(path);
path = Path::simplifyPath(path);

if (FileLister::isDirectory(path)) {
Copy link
Owner

Choose a reason for hiding this comment

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

hmm.. sorry. I think I gave the wrong advice about putting folderExists in Path. It would be preferable to keep the old code in FileLister and reuse FileLister::isDirectory. Sorry I should have realized this from the start.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted my changes FileLister:fileExists Windows implimentation was broken i fixed it.

mSettings->plistOutput += '/';

if ((mSettings->plistOutput != "./") && (!Path::folderExist(mSettings->plistOutput)))
std::string plistOutput = Path::toNativeSeparators(mSettings->plistOutput);
Copy link
Owner

Choose a reason for hiding this comment

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

the scope of this variable can be reduced and it can be const.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it cannot it should be used in the FileLister::isDirectory check. Changed it to const.

This is done in order to support files with the same in diffrent
directories. Added simple cli test for --plist-output cmd.
Added fix for #8289 an error is generated if the --plist-output
folder does not exist.
This is done in order to support files with the same in diffrent
directories. Added simple cli test for --plist-output cmd.
Added fix for #8289 an error is generated if the --plist-output
folder does not exist.

void fileExists() const {
ASSERT_EQUALS(false, FileLister::fileExists("lib"));
ASSERT_EQUALS(true, FileLister::fileExists("readme.txt"));
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove this assertion. The testrunner binary is placed in different paths.. we cant expect that there will be a readme.txt in the build output folder.

The previous assertion is fine. If testrunner is placed in cppcheck root path then it checks that fileExists returns false for a existing folder. Otherwise the assertion will check that fileExists returns false for a non-existing path.. that is good also. The problem would be if the build output folder has a file with the name "lib" but we can maybe ignore that problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We check if we are in the correct place during the test setup see line 41 of testfilelister.cpp
I can still remove it but for me it seems like a valid test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants