lmms icon indicating copy to clipboard operation
lmms copied to clipboard

Add check-namespace script and rewrite verify script

Open JohannesLorenz opened this issue 2 years ago • 3 comments

This adds a (very simplified) namespace/ifdef checker. Also, this puts check-strings and the new check-namespace together.

Obviously, this PR fails its own check-namespace action, until PR #6174 will have been merged.

JohannesLorenz avatar Jun 18 '22 12:06 JohannesLorenz

:robot: Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! :tophat:

Windows

Linux

:robot:
{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/1f269ab8-5091-4efa-96c2-1714d041730e/artifacts/0/lmms-1.3.0-alpha.1.196+g9017775ce-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17974?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/bc81ba11-90af-47d4-9e95-b56c825210a1/artifacts/0/lmms-1.3.0-alpha.1.196+g9017775ce-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17977?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://output.circle-artifacts.com/output/job/2bb5d1a2-0877-407f-ad1e-d126085d86eb/artifacts/0/lmms-1.3.0-alpha.1.196+g9017775ce-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17975?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "1e52563edc3d03795194b462110dd6d4e58c47d8"}

LmmsBot avatar Jun 18 '22 12:06 LmmsBot

@allejok96 I merged your rewrite. It passes most tests, but sadly, it can not detect if the end comment of a macro/namespace does not match the start:

#ifdef XXX
#endif // XXX
namespace ABC {
} // namespace ABC

Do you think you could add this? Or should we ignore this for now?

JohannesLorenz avatar Aug 09 '22 18:08 JohannesLorenz

Huh, I designed it with that in mind, apparently I failed :) maybe I'll have a little time this weekend to look at it.

allejok96 avatar Aug 10 '22 07:08 allejok96

Huh, I designed it with that in mind, apparently I failed :) maybe I'll have a little time this weekend to look at it.

Thanks for the offer. I just fixed the verify script ot let it run again (temporarily). It might be easier for you if you rebase on my changes.

JohannesLorenz avatar Aug 13 '22 17:08 JohannesLorenz

I just tested it on your branch, it works as intended. I never told you how it works though. Feel free to add more comments in the code. Here is an example file you might test against:

#ifndef HEADERGUARD

// Pass - short with no ifdef inside
#ifdef SHORT_MACRO
#endif

// Fail - nested ifdef
#ifdef BAD_MACRO
#ifdef SHORT_SUB_MACRO
#endif
#endif

// Fill with 30 blanks and it will fail - too long
#ifdef BAD_LONG_MACRO
...
#endif

// Pass - no braces inside
namespace GoodNamespace {
class Example;
}

// Fail - contains braces
namespace BadNamespace {
class LongExample
{
public:
    LongClass() {}
};
}

// Pass (if file name is *.h) - first ifndef is concidered header guard and comment is not enforced
#endif 

allejok96 avatar Aug 14 '22 20:08 allejok96

@JohannesLorenz I opened a PR to fix a few edge cases, like } inside a comment

allejok96 avatar Aug 15 '22 20:08 allejok96