Frama-C / headache

Lightweight tool for managing headers in source code files. It can update in any source code files (OCaml, C, XML et al).
Other
24 stars 7 forks source link

Allow skipping more than one line before putting the header #1

Closed vprevosto closed 3 years ago

vprevosto commented 3 years ago

In some settings, more than a single line must stay at the top of the file. The current version of headache only allows skipping the very first line. With this PR, headache will only insert the header after having skipped all the first lines that match the skip regex.

pbaudin commented 3 years ago
  1. the documentation has to be updated
  2. since that changes the actual behavior of the skip directive, it would be preferable to add an optionnal parameter to the skip directive (such as multiline:"true" or similar) to get the new behavior.
vprevosto commented 3 years ago

Fair enough for 1., but I'll actually do it when 2. is settled 😁. Regarding 2, I'm ok with adding a multiline: true or multiline: false directive (I tend to think that "true" wouldn't be very idiomatic). I'd rather make true the default, though, as the cases where it needs to be false seem pretty rare (you would have to have a file beginning with two exactly identical lines, and need to skip exactly the first line, but not the second, before inserting the header).

vprevosto commented 3 years ago

Actually, there's an issue with the multiline directive. What should we do with

".*\.fo." -> skip match: "foo.*" multiline: true
".*\.f.o" -> skip match: "bar.*" multiline: false

and file f.foo

foo matches first directive
bar matches second directive, which has a multiline:false

(same question if we reverse the two lines)

pbaudin commented 3 years ago

Looking just at one entry, it is already possible to write (even if that is not explain into the documentation)

|  ".*\.fo." -> skip match: "foo.*" match: "bar.*" 

which is equivalent to

|  ".*\.fo." -> skip match: "foo.*"
|  ".*\.fo." -> skip match: "bar.*" 

So, I see multiline parameters as a modifier of the next match parameters. The entry

|  ".*\.fo." -> skip match: "bar.*" multiline:"true" match: "foo*"  multiline:"false" match: "...bar*" 

could be equivalent to:

|  ".*\.fo." -> skip multiline:"false" match: "bar.*" 
|  ".*\.fo." -> skip multiline:"true"  match: "foo*" 
|  ".*\.fo." -> skip multiline:"false" match: "...bar.*" 

The default value (corresponding at the original behavior) of the multiline parameter is used for the first entry, and the tool can warn if there is no match parameter after a multiline parameter.

In this example the text "foobar" matches the two last entries. In order to solve such a conflict, the ordering of the rules have to be considered.

The same kind of ordering consideration can be used for @vprevosto example, as it should be the case for the model entries.

vprevosto commented 3 years ago

The default value (corresponding at the original behavior) of the multiline parameter is used for the first entry, and the tool can warn if there is no match parameter after a multiline parameter.

Are you saying that skip multiline: false match: "..." and skip match:"..." multiline: false could have different meaning (depending on the choice of the default for multiline)? This seems extremely counter-intuitive to me. I'd prefer allowing at most one multiline parameter per line of directive, that would apply to all match parameters on the line.

Syntactic consideration apart, I don't feel like this answers my question. I'll try to express it more explicitely: If we have a directive with multiline:true that matches the first line and a directive with multiline:false that matches the second line, do we put the header before or after the second line? Same question arises if we have instead a directive with multiline:false matching the first line and a directive with multiline:true matching the second line.

pbaudin commented 3 years ago

After a careful look at the original algorithm, the order of the skip entries and the one of the matching parameters do not affect the result. @virgile, it is also the case with your proposed commit 5317b25. Now, I better understand your question.

I can suggest an alternative that keeps this property in giving for example a higher priority the regexp for mutiple lines.

The original grammar

| <file-regexp> "->" "skip" ("match" ":" <line-regexp>)*

of the skip entries can be extended to

| <file-regexp> "->" "skip" (("match"|"multiline_match") ":" <line-regexp>)*

With that "multiline_match" parameter, the semantic is easy to understand, since the order of the parameters has no importance.

The skip algorithm can iterate on the first lines until the checked line matches a multi-line regexp. Otherwise, the iteration stops in removing the last checked line when that line matches a non multi-line regexp.

@virgile, with such a algorithm, the answer to your last questions would be:

vprevosto commented 3 years ago

OK, let me rephrase your last points to ensure that we are on the same page: as long as the lines match a multiline_match regex, we skip them and check the next line, and if we only have a match that matches, we skip the current line and break the iteration there (of course, if nothing matches, we put the header before the current line). That's fine with me.

pbaudin commented 3 years ago

Fine. I will try to use as much as possible your rephrasing for the documentation update.

No objection to the name of the new parameter?

vprevosto commented 3 years ago

No objection, multiline_match seems fine.

pbaudin commented 3 years ago

@maroneze, could you review the documentation updates?

maroneze commented 3 years ago

There are a few typos (1 new, several pre-existing) that I'd like to fix. Should I rebase my commit before the one changing the version number? Or just add a new commit?

pbaudin commented 3 years ago

Thanks @maroneze. You can add your commit. It may include the fix about the frame character that is defined for the swi-prolog files.

pbaudin commented 3 years ago

Then I will re-generate the published documentation and rebase the branch before merging.

maroneze commented 3 years ago

I committed changes to doc-src/manual.tex.