Skip to content
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

Jump to bottom

addons/namingng.py: Improve file name checking feature. #5802

Merged
merged 1 commit into from
Dec 27, 2023
Merged

addons/namingng.py: Improve file name checking feature. #5802

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 9 additions & 4 deletions addons/namingng.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

import cppcheckdata
import sys
import os
import re
import argparse
import json
Expand Down Expand Up @@ -96,10 +97,14 @@ def process(dumpfiles, configfile, debugprint=False):

# Check File naming
if "RE_FILE" in conf and conf["RE_FILE"]:
mockToken = DataStruct(afile[:-5], "0", afile[afile.rfind('/') + 1:-5])
msgType = 'File name'
for exp in conf["RE_FILE"]:
evalExpr(conf["RE_FILE"], exp, mockToken, msgType, errors)
for source_file in data.files:
basename = os.path.basename(source_file)
good = False
for exp in conf["RE_FILE"]:
good |= bool(re.match(exp, source_file))
good |= bool(re.match(exp, basename))
Copy link
Owner

Choose a reason for hiding this comment

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

it's not very obvious to me what basename does.. can you give an example why you want to match both source_file and basename?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certainly, there are multiple use cases. For clarity: the original code attempted to distill the basename from the dump filename (by removing the .dump suffix) and only tested that. With this patch the code tests filenames of all files, also including header files. Both the basename and the full path (source_file) are tested, which gives some added flexibility.

On my system I get file naming violations on system includes, such as /...../gcc-arm-none-eabi-10.3-2021.10/arm-none-eabi/include/sys/_sigset.h that don't follow my rules because the basename starts with _. Of course I can make a suppression for it, but a different approach is to make a pattern that matches and accepts such filenames. In the example this is any absolute path ("/.*\\.[ch]\\Z") but it may also be based on a particular path (".*arm-none-eabi/include").

Another use case is where under a particular path a different convention is followed. Again from own experience, when developing for embedded systems we get a lot of boilerplate, e.g. CMSIS and the STM32 HAL, that we don't necessarily want to completely ignore or rename. So we can make a few different rules, such as "Drivers/CMSIS/<insert regexp>", "Drivers/STM32[^/]*/<insert regexp>", etc.

Yet another use case is that this allows to validate the naming and structure of directories as well, e.g. by having all RE_FILE rules prefixed, like "[A-Z][a-z]*/[a-z][a-z0-9_]*[a-z0-9]\\.[ch]\\Z" which would allow "Core/file_1.c" but not "tmp/hack.c" or "Core/tmp/quickfix.c" or "HEADERS/hdr.h".

Of course there are various other ways to solve such cases, where different paths need different rules, or where a different tool should validate directory naming and structure, but as we're validating the filename of all files, it seemed natural to (ab)use RE_FILE for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ps. I will add a unit test, but that depends on the namingng CLI patch going through first.

if not good:
errors.append(reportError(source_file, 0, 'style', 'File name ' + source_file + ' violates naming convention'))

# Check Namespace naming
if "RE_NAMESPACE" in conf and conf["RE_NAMESPACE"]:
Expand Down
Loading