Created on 2023-01-10.19:57:56 by florian, last changed by florian.
File name |
Uploaded |
Type |
Edit |
Remove |
help.t2t.issue1073
|
florian,
2023-01-13.21:41:10
|
application/octet-stream |
|
|
help.t2t.main
|
florian,
2023-01-13.21:40:50
|
application/octet-stream |
|
|
help.txt.issue1073
|
florian,
2023-01-13.21:41:46
|
application/octet-stream |
|
|
help.txt.main
|
florian,
2023-01-13.21:41:29
|
application/octet-stream |
|
|
msg10931 (view) |
Author: florian |
Date: 2023-01-18.18:16:18 |
|
Thanks. The style checks indeed fail because of the indentation of the enum definitions. Since this is a temporary thing, I hope it is ok to make an exception here.
I merged the issue and all other checks passed, so I'll close this one and rebase issue1040.
|
msg10929 (view) |
Author: salome |
Date: 2023-01-17.19:20:40 |
|
Thanks for the answers, sounds all good to me! I looked at the changes (also the new string utility function) and from my side this looks ready to merge. The only caveat is that the style checks fail, but I don't know if this is a known issue (I didn't relly follow the recent problems with Github actions and clang and whatnot).
|
msg10928 (view) |
Author: florian |
Date: 2023-01-17.16:59:26 |
|
Thanks for the review Salome. I handled your comments and pushed the changes.
Regarding 1.
If we remove the alias, we have to add plugins:: in many more places that will all disappear in the merge, so we kept the alias for now and only changed the places that will remain. In issue1040 the alias is removed so Options no longer is a valid type and all code has to either use plugins::Options or introduce the alias themselves.
Regarding 2.
You are right, this doesn't make sense. I undid the change here and in issue1040.
I added one more string utility function that I missed the first time.
|
msg10927 (view) |
Author: salome |
Date: 2023-01-17.14:52:58 |
|
I had a look at the pull request and added some comments/questions. I want to reiterate two of my questions here though since they might be worth discussing a bit more in-depth:
1) Sometimes you add "plugins::" in front of Options or similar objects (when "options::" was not there before) but plugins/plugin.h still has "using plugins::Options;". Is the idea to eventually always explicitly write "plugins::" and drop the "using" statements? If so why can't we do that yet?
2) Changing how the description is set in Heuristic/Evaluator looks like a rollback from issue921, is that intended?
I also wrote down a somewhat more detailed summary of the changes, so I just add it here in case someone wants to read it:
- rename folder options to plugins
- remove option_parser/option_parser_util/plugin (which contained includes and using ... statements) and move their content to plugins/plugin.h
- remove "using plugins::Bounds" and instead write "plugins::Bound" everywhere directly
- reorder code in parse methods to make it consistent with wiki order (synopsis, options, notes, language support, properties)
-> fixed a bug in h^m where existing documentation was not shown
- indent content of parse methods and surround them with brackets (the indent will be needed for issue1040)
- For each enum, a static method is implemented that hardcodes the enum. Additionally the enum is now a vector of pairs of strings (name,desc) instead of two vectors of strings.
- enums use lower case, add enum documentation
- remove declaration "static OpenList<Entry> *parse(...)", it was never implemented
- Refactor parse function in potential heuristic. Previously a _parse function was called by two specialized parse function. The _parse function however only called another function, created the opts object and returned the heuristic (or nullptr). These are now directly implemented in the two specialized parse functions, leading to some code duplication but in my opinion the code is clearer now.
- description for heuristic and search engine is now set differently
- program name is stored globally instead of passing it to the usage() function
- the parser complains if two --search options are given (previously it would overwrite the first one)
- Exception base class now has a field msg and get_message() and print() functions
- string utilities: added "to_lower" (returns lowercase copy of the string), changed "split" to return vector of first n splits for parameter n (-1 for unlimited). If delimeter is not found, the vector contains one entry with the entire string.
|
msg10926 (view) |
Author: florian |
Date: 2023-01-13.21:52:37 |
|
I created the documentation in both formats for the base code and the issue code, in case someone wants to look at a diff.
Here is an overview of the changes:
* We converted all enums values to lower case.
* We added some documentation for enums values and enum options. We also removed some documentation from the options when it was redundant with the documentation of the values.
* The order of some notes changes. This is a side effect of us ordering the definitions. Some notes are added within add_*_option_to_parser() functions and moving these to where the other options are added, changes the relative order of the notes (the overall order of options before notes is unaffected).
* Some documentation we already had was "hidden" after a "if (helpmode) return;" statement and were never generated so far.
* Some documentation was wrong and we fixed it.
|
msg10925 (view) |
Author: florian |
Date: 2023-01-13.21:34:36 |
|
I set up a pull request for this:
https://github.com/aibasel/downward/pull/142
The style checks fail for the way the enums are written but this will change in issue1040 and the current indentation matches that pull request, so I'd leave it like this. I ran uncrustify locally to verify that there are no other issues.
|
msg10909 (view) |
Author: florian |
Date: 2023-01-10.21:42:49 |
|
Actually point 1 is not possible. I tried this in a file without forward declarations before posting and it worked there. However, we forward declare Options a lot and it turns out you cannot forward declare an alias. My new suggestion is to actually move the whole "options" directory to plugins. The new code (in issue1040) deletes the options directory anyway and the files that we keep end up in the directory "plugins", so this will also make the diff for those files smaller.
|
msg10907 (view) |
Author: florian |
Date: 2023-01-10.19:57:55 |
|
Issue1040 touches a large amount of files. While working on this issue, we also did some changes that also could have been done independently of the issue. This makes the diff of issue1040 very noisy and it is not that easy to see what actually changed functionally. Here, I would like to do some of the non-functional changes in an effort to reduce the diff of issue1040. This is a bit mix of different things that normally wouldn't fit in one issue, but I think it makes sense to make an exception here, so we can hopefully review and merge issue1040 soon (ideally before the next sprint).
On the list of changes, I'd like to do here:
1. The namespace of the class Options will change from `options` to `plugins`. Also most plugins currently include "../option_parser.h" and "../plugin.h" and in the future, they will have to include "../plugins/plugin.h". I would create a file "../plugins/plugin.h" that includes the other files and contains:
namespace plugins {
using Options = options::Options;
}
This way, all imports can already be fixed and all "options::Options" can already be changed to "plugins::Options" where the class will live in the future. There are at least 20 files where this is the only change, so this will already reduce the noise in issue 1040.
2. We decided to change the "canonical way" to write enums in lower case (they are case insensitive and currently occur in both cases in the code). I suggest we do all transformations to lower case here, so this does not show up in the diff of the code or in the diff of the documentation.
3. When defining options, notes, supported language features, and properties, there is currently not a consistent order but the most common order does not agree with the order in the wiki. We decided that we want the use a consistent order in the code as well that matches the wiki order (title, synopsis, options, notes, language support, properties). Doing this here will also simplify the diff.
4. We fixed some documentation and I think we also added some. I'd include this here as well.
5. This might be a bit controversial but I'd also like to indent most things in the parse function one additional level. This doesn't make sense in the current code but due to the way the new option parser works, the corresponding lines in the new code are all indented further and Github cannot match them easily. This leads to large blocks of text showing up as modified even if all changes are whitespace. I'd add a {...} around the relevant code, so uncrustify doesn't complain. Another way of doing this would be to create a temporary branch and indent the code there, then view the diff to that branch. But this would make things like commenting on the pull request more complicated.
|
|
Date |
User |
Action |
Args |
2023-01-18 18:16:18 | florian | set | status: chatting -> resolved messages:
+ msg10931 |
2023-01-17 19:20:40 | salome | set | messages:
+ msg10929 |
2023-01-17 16:59:26 | florian | set | messages:
+ msg10928 |
2023-01-17 14:52:58 | salome | set | nosy:
+ salome messages:
+ msg10927 |
2023-01-13 21:52:37 | florian | set | messages:
+ msg10926 |
2023-01-13 21:41:46 | florian | set | files:
+ help.txt.issue1073 |
2023-01-13 21:41:29 | florian | set | files:
+ help.txt.main |
2023-01-13 21:41:10 | florian | set | files:
+ help.t2t.issue1073 |
2023-01-13 21:40:50 | florian | set | files:
+ help.t2t.main |
2023-01-13 21:34:36 | florian | set | messages:
+ msg10925 |
2023-01-10 21:42:49 | florian | set | status: unread -> chatting messages:
+ msg10909 |
2023-01-10 19:57:56 | florian | create | |
|