Repository navigation
Norm 4.11 and the remaining open issues - #598
Open
wil-pedago wants to merge 53 commits into
Open
wil-pedago wants to merge 53 commits into
wil-pedago wants to merge 53 commits into
Conversation
grid[size + i++ * (size + 1)] = 'x'; reported SPC_AFTER_POINTER. Walking left from the asterisk, `is_operator` reached the `++`, found it in the operator list and concluded the asterisk was a dereference. A postfix increment leaves a value behind it, so what follows is an operator. The increment has to be postfix for that to hold, which is checked by looking at what sits before it. `*p`, `++*p` and `*p++` still read as pointers, and `i = * p;` still reports SPC_AFTER_POINTER. Closes #303
The notice said "Make sure it is a reasonable choice", which states nothing. The norm says a global that is not const or static is forbidden, unless the project explicitly allows it, so the message says that instead. The sample outputs and the six locale files are regenerated. The translation of this one message is empty in all of them until the translators get to it.
int ft_count_if(char **tab, int(*f)(char *)); reported MISSING_IDENTIFIER, while the same line with a space after `int` did not. The norm exempts type keywords from the rule asking for a space after a keyword, so both spellings are allowed. `check_type_specifier` returns the last token it consumed, and it does that by stepping back one. With a space after the type that lands on the space, which the caller then skips. Glued, it lands back on the type itself and the identifier is never found. It now steps back only when there is something to step back onto. An argument that really has no name still reports MISSING_IDENTIFIER, `int(*)(char *)` and `int` included. Two sample outputs move a column: the error now points at the type without a name rather than at the space or parenthesis in front of it. Closes #306
enum e_actions { action_init, last_action };
t_fn g_tab[last_action];
reported VLA_FORBIDDEN. The check treats any array size holding a
lowercase letter as a variable, which reads a macro apart from a
variable but not an enum constant. The norm asks for lowercase
identifiers, so an enum constant looks exactly like a variable to that
rule.
The constants declared in the file are now collected as the enum is
parsed, and an array sized with one of them is left alone.
`char str[argc]` and `char str[n]` still report VLA_FORBIDDEN.
An enum coming from an included header stays unknown, since norminette
reads one file at a time.
Closes #278
#define LEN 8
typedef char t_buf[LEN];
reported FORBIDDEN_CHAR_NAME on `LEN`. `IsUserDefinedType` collects
every identifier in the statement and keeps the last one as the name.
With an array size that last identifier is the size, not the name, and
a macro is uppercase, so the name check rejected it.
Identifiers inside brackets are no longer collected. `t_Bad[8]` still
reports FORBIDDEN_CHAR_NAME.
`test_file_210218_3.out` held that false positive as expected output,
so it is regenerated.
Closes #574
if (get_res((t_stct){.i = v1, .j = v2}, 1) == 1)
reported ASSIGN_IN_CONTROL. The scan for an assignment inside the
condition follows the parenthesis depth but not the braces, so the `=`
of a designated initialiser counted. The positional form of the same
literal was already accepted.
Assignments in braces are skipped now. `if ((a = b) == 1)` still
reports ASSIGN_IN_CONTROL.
The loop also had no end of file guard, so an unclosed parenthesis in
the condition kept it going. It stops at the end of the stream.
Closes #471
#ifndef WRONG_NAME
# define WRONG_NAME
#endif
reported HEADER_PROT_NAME, which is right, and HEADER_PROT_NODEF,
which is not. On `#endif` the rule looked for a macro named after the
file, so a guard that is misnamed also counted as having no define.
It now looks for the name the file used, ignoring case since a guard in
lowercase already reports HEADER_PROT_UPPER. One mistake, one error.
A guard with no `# define` at all still reports HEADER_PROT_NODEF. On a
corpus of 804 real files this drops 34 of these errors, and every one
of those files does have a matching define.
Closes #280
v = 0xb401D3; reported INVALID_SUFFIX, while `0xa401D3` did not. The prefix pattern is `0[bBxX]*`, which is greedy, so it swallowed the `b` that is really the first hex digit. What was left, `401`, parsed as the number and `D3` as a suffix. The pattern now tries `0x` and `0b` before the greedy form. A repeated prefix keeps its old reading, so `0xx12` and `0bb1` are unchanged, and `0b12` still reports INVALID_BIN_INT, `0779` INVALID_OCT_INT. Closes #568
unsigned a;
signed c;
reported IMPLICIT_VAR_TYPE, while `long d;` and `short e;` did not.
`signed` and `unsigned` sit in the list of qualifiers next to `const`,
so they were skipped and the variable name was read as the type,
leaving nothing to name. On their own they mean `signed int` and
`unsigned int`.
A sign specifier is now only skipped when a type follows it, so
`unsigned int b`, `const unsigned d` and `unsigned long e` are
unchanged.
`test_file_0907.out` held the same false positive as expected output,
`ARG_TYPE_UKN` on the argument `unsigned *index2`, and is regenerated.
Closes #508
typedef enum e_type
{
TEX,
UNKNOWN // comment here
} t_type;
produced six errors, none of them about the comment, spread over every
line after the enum and ending with PREPOC_ONLY_GLOBAL on the `#endif`.
The scan for an enum member does not know the comment tokens, so it
gave up on the member, the closing brace was never matched and
everything below moved into the wrong scope. Comments are skipped now,
the way they already are when scanning an ordinary declaration.
Closes #584
Several comments on one line multiplied their errors:
int a; /*1*/ /*2*/ /*3*/ /*4*/ /*5*/
gave twenty errors for five comments, the first reported twice and the
last six times. `CheckComment` runs after every rule that matches on
the line and scanned to the end of the line each time, so a comment was
reported once per pass.
The scan stops at the end of the statement that matched. On a corpus of
804 real files this removes 60 duplicate errors across 16 files, and no
comment loses its error: the set of reported positions is unchanged.
Three sample outputs held the duplicates as expected output and are
regenerated.
Closes #596
A comment running past eighty columns produced two LINE_TOO_LONG for the same line, one from the comment rule and one from the line rule, which each keep their own idea of what they have already said. The line rule now looks at what has been reported before adding its own. On a corpus of 804 real files this removes 60 duplicates across 10 files, and no line loses its error. Whether a comment should count towards the line length at all is a separate question. The norm sets a limit on lines without exempting comments, so it does. Closes #433
if (pthread_create(&philos[i].thread, NULL, \
check_status, &philos[i]))
reported MIXED_SPACE_TAB on the first line and TAB_INSTEAD_SPC on the
second. The norm asks for exactly that indentation: an instruction
split over several lines must have the following ones indented.
The lexer drops the escaped newline, so the tabs opening the second
line land in the token stream right after the space before the
backslash, and both spacing rules read them as tabs in mid line. They
now recognise the run of tabs that opens a line and leave it to the
indentation rules.
A tab in the middle of a line still reports TAB_INSTEAD_SPC.
`test_file_210223.out` held both false positives as expected output and
is regenerated.
Closes #558
Two asks that were easier to grant than to argue about. `--only-errors` drops the files that have nothing to report, so a run over a project prints what needs fixing and nothing else. The exit status does not change, and `--format json` keeps every file, since something reading the JSON can filter it and may count on one entry per input. `-` as the file argument reads one file from standard input, with `--filename` naming it so the header check has something to compare against. `--cfile` already carried content, but through argv, which means shell quoting and a size ceiling. Closes #578 Closes #364
while (a)
if (a)
{
a++;
a++;
}
reported MULT_IN_SINGLE_INSTR. The rule contradicted the docstring
above it, which says a control structure without braces "can only
contain one instruction line, but can contain multiple control
structures". It fired on exactly that case.
The check is removed. A control structure counts as one instruction, so
holding another one, braced or not, is allowed. On a corpus of 804 real
files this drops 32 errors and nothing else moves.
The wording in the norm that this reads from is ambiguous, "a single
instruction on a single line", and deserves the same clarification. The
norm source lives in 42born2git, so that goes in its own change.
Closes #320
The norm forbids a global that is not const or static, "unless the project explicitly allows them". There was no way to say a project does, so every such project carried the notice on every file. `--allow-globals` drops GLOBAL_VAR_DETECTED. It keeps GLOBAL_VAR_NAMING, since a global that is allowed still has to start with `g_`. Closes #516
The norm calls a global "a norm error, unless the project explicitly allows them", and norminette reported it as a notice, which passes. It is an error now, and `--allow-globals` is the way to say a subject permits them. The exemption for const and static is gone with it. Most projects have no reason to carry a global at all, so the rule is the same whatever the form: `static int g_x;`, `const int g_x;` and `const void *g_x` are all reported. That last one was never const anyway, only the data it points at was. GLOBAL_VAR_NAMING is unchanged, a global that a subject allows still has to start with `g_`. On a corpus of 804 real files, 22 hold a global and none of them changes verdict: they were all failing on something else already. Closes #285 Closes #337
The check accepted one literal, optionally signed, and nothing else, so `(A + 1)` and `60 * 60` were reported. The norm allows a define to hold "literal and constant values", and a constant expression is a constant value. The value is now read as an expression: operands and operators alternating, with balanced parentheses. `1 + 2`, `(1 << 8) / 8` and `AB+AB` pass. `1 1 1`, `&&`, `TATA 12` and `(1 + 1` do not, since none of them is an expression, and neither do `foo(1)` and `a = 1`. A define with no value at all, a header guard, is left alone. Macro functions keep MACRO_FUNC_FORBIDDEN and a ternary keeps TERNARY_FBIDDEN. On a corpus of 804 real files this accepts 13 defines that were reported, `(17 * 2)` and `60 * 60 * 24` among them. Closes #534
`typeof` is a GNU extension that C23 standardised, and the norm does not allow it. It was not recognised at all, so a declaration using it collected errors about tabs and empty lines that said nothing about the cause. A dedicated check names it. An identifier that merely contains the word, `typeofish`, is left alone, and `__typeof__` and `typeof_unqual` are reported too. Closes #443
The norm allows one instruction per line. A comma directly between the parentheses of a return separates two of them, while a deeper comma belongs to a call and is left alone. An operator opening a line split on a backslash can have nothing before it, so stop asking for a space there. Closes #305 Closes #355 Closes #375 Closes #497
A function returning a function pointer had only the argument list of the returned type checked, so an empty one on a parameter went through. Every parenthesis pair of a header is an argument list, so look at them all. Closes #335
A declaration with no type matched no rule, so the parser read its body as global scope and reported indentation errors on it instead. Closes #363
A newline between the two was walked over, so nothing separated them and no error came out. Closes #527
Only a malformed protection was reported, so a header with no #ifndef went through. Closes #342
The scan stopped at the first newline, so a comment on a continuation line was never seen. Whether a comment sits in the middle of something is now judged on its own line. Closes #288
The scan jumped over a parenthesis group whose contents it could not name, and a cast in front of the assignment was enough for that. Closes #467
The pattern accepted an empty file name, author or date, so a header with the right shape and nothing in it passed. Closes #334
The spacing rule gave up on preprocessor statements to leave their space indentation alone, and dropped the trailing check with it. Closes #396
New checks make code that passes today fail, so this is not a patch.
Each test states what is reported and what stays quiet, so a regression names the rule it broke.
The Norm forbids them, and nothing checked it: a define carried over a backslash was reported as bad indentation, which no indentation fixes.
The marker says the norminette does not verify a rule. It verifies this one, and 4.11 widened it to constant expressions.
This was referenced Sep 1, 2026
The Norm asks it of files and directories and carries no (*), so it says the norminette verifies it. It did not look at the name at all. Directories are read only from a relative path, the ones above a project being none of its business.
Rebuilt from the same class file the previous PDF came from, so the only differences left are the 4.11 wording itself.
Seven new or reworded errors, in the five translated locales, following the terms each catalog already uses.
The same decisions as the English document, plus three errors the translations carried: - fr forbade globals "except when you have to (signal handling)", a rule the English never had - es and tr read "const and static" where the English reads "or", which exempts a variable marked only const - pt_br carried a (*) on the define rule, which the norminette checks fr had no rule on braces around a control structure at all, so that one is written rather than amended. ja and ko need texlive-lang-japanese and texlive-lang-korean to build, which this machine has not, so they stay at 4.1 rather than ship a tex and a pdf that disagree.
Their babel language files are not installed here, and nonstopmode had let the build emit a pdf anyway, without the hyphenation and typography of the language. fr and tr pass the reference makefile, which halts on error, so they stay.
wil-pedago
requested review from
boulon42,
kcp2p and
malallai
and removed request for
malallai
September 1, 2026 13:48
malallai
reviewed
Sep 21, 2026
malallai
left a comment
malallai
left a comment
Collaborator
There was a problem hiding this comment.
Looks fine to me, I didn't find anything wrong from my POV.
kcp2p
previously approved these changes
Sep 21, 2026
kcp2p
left a comment
kcp2p
left a comment
Collaborator
There was a problem hiding this comment.
LGTM aftter thorough testing
|
Tested this branch against #596 (n stacked One thing I found while profiling: there is still a quadratic cost on this branch, and its main source is introduced by 8ec148e (#433, "report a line as too long once"). Cause. Profile at n=5000 (33.8 s under cProfile): Fix.
Output is byte-identical before/after on n=2500/5000/10000 and on all 292 files in Patch (Errors line index + CheckLineLen)diff --git a/norminette/errors.py b/norminette/errors.py
index dab6dd9..f924190 100644
--- a/norminette/errors.py
+++ b/norminette/errors.py
@@ -10,6 +10,8 @@ from typing import (
Literal,
Optional,
List,
+ Dict,
+ Set,
overload,
Any,
Type,
@@ -92,10 +94,13 @@ class Error:
class Errors:
- __slots__ = "_inner"
+ __slots__ = ("_inner", "_lines")
def __init__(self) -> None:
self._inner: List[Error] = []
+ # Lines already reported, by error name. Rules ask this on every run,
+ # so it is kept up to date here instead of rescanning `_inner`.
+ self._lines: Dict[str, Set[int]] = {}
def __repr__(self) -> str:
return repr(self._inner)
@@ -155,8 +160,15 @@ class Errors:
if len(args) == 2:
error = Error(*args, **kwargs)
assert isinstance(error, Error), "bad function call"
+ lines = self._lines.setdefault(error.name, set())
+ lines.update(highlight.lineno for highlight in error.highlights)
return self._inner.append(error)
+ def lines_with(self, name: str) -> Set[int]:
+ """Line numbers highlighted by the errors named `name` added so far.
+ """
+ return self._lines.get(name, set())
+
@property
def status(self) -> Literal["OK", "Error"]:
return "OK" if all(it.level == "Notice" for it in self._inner) else "Error"
diff --git a/norminette/rules/check_line_len.py b/norminette/rules/check_line_len.py
index 37e3681..5a60950 100644
--- a/norminette/rules/check_line_len.py
+++ b/norminette/rules/check_line_len.py
@@ -6,12 +6,7 @@ class CheckLineLen(Rule, Check):
"""
Lines must not be over 80 characters long
"""
- reported = {
- highlight.lineno
- for error in context.errors
- if error.name == "LINE_TOO_LONG"
- for highlight in error.highlights
- }
+ reported = context.errors.lines_with("LINE_TOO_LONG")
for tkn in context.tokens[: context.tkn_scope]:
if tkn.pos[1] > 81 and tkn.pos[0] not in reported:
context.new_error("LINE_TOO_LONG", tkn)Remaining. Beyond ~10k comments a smaller quadratic part is left (n=20000: 2.95 s, n=40000: 8.72 s): |
Collaborator
Author
|
Thanks a lot @veliga-syutkin, good catch. Confirmed, and it was 8ec148e indeed. Your patch is in (bdb8b1f, credited as co-author). On my machine it gave about x3 though, not x25: With both, n=5000 goes from ~15 s to ~1.3 s and n=10000 runs in ~3 s. Output is identical on your repro and on all the samples, and the 605 tests pass. |
CheckLineLen runs once per comment and walked context.errors each time, which sorts the whole list. With n comments on one line that is n sorts of about n errors (#596). Errors now keeps the reported lines by error name, updated in add(). Co-authored-by: veliga-syutkin <144716057+veliga-syutkin@users.noreply.github.com>
is_inside_a_function walked the whole history back for each comment, the other quadratic cost of #596. The depth of the last function body is now kept on the context and only fed the records added since the last call.
wil-pedago
force-pushed
the
fix/remaining-issues
branch
from
October 7, 2026 07:31
d8b9c29 to
12ef318
Compare
Collaborator
Author
|
@veliga-syutkin about |
…prit A file's verdict depended on how its path was typed: 'norminette Libft/' flagged every file because of 'Libft', an absolute path checked no directory at all. Directories are now read relative to the folder the run starts from (the folder given, or the current one for a bare run), never that folder itself. A file named on the command line is judged on its own name. The message says which file or directory name is wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows #595. Brings the Norm to 4.11 and closes the issues that were left open.
The version goes to 3.4.0: the last two sections make code that passes today fail.
Parser fixes — 21 issues
norminette reported valid code, or reported one mistake several times.
52eaeea32a370709c85d53e60ddd71e4ddf5b64219073f5f2425add0d38a5eed0b539c8ec148e165ba97c4258d66ce4a9cd29956ae77ab7db3501744c054daad2839bNorm 4.11 — 13 issues
The decisions behind the rules, written down in
pdf/en.norm.tex, and followed by the rules.d13c9e08933ea999c0b54b22f8d128a78b91360717a5fcca3f1b0f03Checks that were missing — 15 issues
Rules the Norm states and norminette never verified.
f53dd694017cbcffebc21664c510ff5b070a1702a04b0d66e369b2d1d2cd47fd673bfde4f335bTests
33 samples and 28 regression tests, each stating what is reported and what stays quiet.
Coherence pass
Every rule of the document was run against the tool, one conforming and one offending case each: 59 rules, all consistent. It turned up two rules the Norm states and nothing verified, both now checked:
Directory names are read only from a relative path, the ones above a project being none of its business.
Norm documents
en,frandtrare at 4.11, tex and pdf. The pass over the translations also turned up three errors that predate this work:fres,trconstandstaticconstorstatic, so a variable marked onlyconstwas exemptpt_br(*)on the#definerulefralso had no rule at all on braces around a control structure.es,pt_br,jaandkostay at their current version: their babel and CJK language files are not available here, and the build emits a pdf anyway, without the hyphenation and typography of the language. Shipping a tex and a pdf that disagree would be worse than leaving them behind.Messages
The seven new or reworded errors are translated in the five translated locales, following the terms each catalog already uses.
jaandarwere written by pattern and deserve a native reader.Closes
Closes #277
Closes #278
Closes #280
Closes #285
Closes #288
Closes #290
Closes #302
Closes #303
Closes #305
Closes #306
Closes #315
Closes #317
Closes #320
Closes #330
Closes #333
Closes #334
Closes #335
Closes #337
Closes #342
Closes #355
Closes #359
Closes #363
Closes #364
Closes #366
Closes #375
Closes #394
Closes #396
Closes #433
Closes #443
Closes #461
Closes #467
Closes #468
Closes #471
Closes #473
Closes #497
Closes #504
Closes #508
Closes #516
Closes #527
Closes #529
Closes #534
Closes #557
Closes #558
Closes #568
Closes #574
Closes #578
Closes #580
Closes #584
Closes #596