You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/03/14 21:22:45 UTC
[GitHub] [incubator-nuttx] johannes-nivus opened a new pull request #569: Add possibility to add identifier whitelist to block comments of file…
johannes-nivus opened a new pull request #569: Add possibility to add identifier whitelist to block comments of file…
URL: https://github.com/apache/incubator-nuttx/pull/569
… under test.
Attention this is **experimental**.
This PR is not supposed to be merged but a base for discussions on whitelisting inside the file under test.
The control comments could perhaps be used for some more, e.g.:
```
<nxs ignore="1" />
```
This could ignore file at all if used early (right after license header).
For sure, usage of these control comments need to be restricted within the coding standard.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569#issuecomment-599213134
The files would be just a list of names like. For ESP32 it would be a list like:
Cache_Flush
Cache_Read_Enable
etc.
(See esp32/esp32-core/scripts/esp32_rom.ld for the whole list).
That ESP32 list would never change.
A highe level whitelist would be just another list of names like
Elf32_hdr
etc. (can't think of more now)
These would change perhaps once every 5 years or so as standards evolve.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569#issuecomment-599211746
> But think of the can of worm on the commit history: Every PR touches these files.
I don't see that. Modifications to the white list file, whatever it is called, would be very rare. Only when a new non-compliant name is added which should be never.
We should not permit people to create new non-compliant names. The existing non-compliant names are a consequence of names defined in other standards and by chip manufacturers. If POSIX changes or if we add support for new chips, then and only then would the whitelists change.
I see no can. I see no worms.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] johannes-nivus commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569#issuecomment-599137458
I forgot: This is in reference to #550
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569#issuecomment-599203986
I am alsi opposed to modyfing source files to support the tool. I don't think we should go that direction.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569#issuecomment-599210825
> Why are you always insisting the tool is broken?
Way, way, way off topic. We should not defocus the discussion with this nonsense. We should stick to the subject at hand.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569#issuecomment-599204148
Please, no mera data in sorce files!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569#issuecomment-599210375
> There are other tools that use metadata inside source files, doxygen for example.
> Why restrict own tools?
Use of Deoxygen tags in source is also forbidden by the coding standard.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] johannes-nivus closed pull request #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
johannes-nivus closed pull request #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] davids5 commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569#issuecomment-599209973
> Why are you always insisting the tool is broken?
Did you see what it do to this poor fellow on this PR. https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597682980
What else would you call it?
Have you worked on a project that uses Astyle or Uncrustify and it just works?
Do not get me wrong I hold NuttX CS as a top priority it has always been my position.
But we need to fix this.
David
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569#issuecomment-599245358
I didn't address some of these
> The tool itself is supposed to traverse the folder structure down to the file under test, looking for .nxignore files, with inheritance?
That would be a nice design. Beware, however, that this needs to run in both Windows and Unix-like native modes. Makefile.host defines -DCONFIG_WINDOWS_NATIVE=y on the command like in the Windows native case. The primary thing is that you need to select the correct path segment delimitor, '\' or '/', depending on that setting.
Functions like basename() should work without modification so traversing up from the file under test would be more portable.
>The top level directory is specified? Or determined from the location of the tool?
The tool could be run from any directory, so that is not useful and the top level directory is not specified. That however, is not a problem if you traverse from the directory containing the file-under-test upward. The top-level directory should contain a landmark like INVIOLABLES.txt or NOTICE (don't depend on .git being present. .git will not be present if users using there own CM system).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569#issuecomment-599204148
Please, no mera data in sorce fikes!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] johannes-nivus commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569#issuecomment-599222572
From a technical point of view:
The tool itself is supposed to traverse the folder structure down to the file under test, looking for .nxignore files, with inheritance? The top level directory is specified? Or determined from the location of the tool?
Should the .nxignore file support wildcards like '*' or '?'
Should the .nxignore file only whitelist symbolnames or complete files as well?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] davids5 commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569#issuecomment-599231310
Ahh OK now I see where was misunderstanding the purpose. In that case I would make a directory under the tool and add the files there.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569#issuecomment-599245358
I didn't address some of these
> The tool itself is supposed to traverse the folder structure down to the file under test, looking for .nxignore files, with inheritance?
That would be a nice design. Beware, however, that this needs to run in both Windows and Unix-like native modes. Makefile.host defines -DCONFIG_WINDOWS_NATIVE=y on the command like in the Windows native case. The primary thing is that you need to select the correct path segment delimitor, '\' or '/', depending on that setting.
Functions like basename() should work without modification so traversing up from the file under test would be more portable.
>The top level directory is specified? Or determined from the location of the tool?
The tool could be run from any directory, so that is not useful and the top level directory is not specified. That however, is not a problem if you traverse from the directory containing the file-under-test upward. The top-level directory should contain a landmark like INVIOLABLES.txt or NOTICE (don't depend on .git being present. .git will not be present if users using their own CM system).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569#issuecomment-599224304
> The tool itself is supposed to traverse the folder structure down to the file under test, looking for .nxignore files, with inheritance? The top level directory is specified? Or determined from the location of the tool?
That would be a nice design, but perhaps overkill for the problems at hand. If you inspired, to implement that, the approach does seem reasonable.
> Should the .nxignore file support wildcards like '*' or '?'
Again, that would be a nice convenience but not essential. There is a simple wildcard string matcher at libs/libc/misc/lib_match.c that you could probably repurpose for this use.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] davids5 commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569#issuecomment-599212036
hmm - perhaps do I not understand what is being proposed.
How about some real world examples of what would be in the files and when it is added?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] davids5 commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569#issuecomment-599211373
>An option would be add a whitelist outside of the source files. Perhaps a file called .nxignore that is in the same directory as the source file. This file could be read, if it exists, to get the whitelisted names.
>A higher level .nxignore, perhaps in tools/, could also be used.
Yeah I thought about that too....
But think of the can of worm on the commit history: Every PR touches these files.
Rebase HELL! Yuch!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] johannes-nivus commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569#issuecomment-599203617
> But I think t is not a good idea to to add the meta data to the source files.
I think the source file itself is the place where this metadata is best kept, because it is self contained.
> Really I think we should fix the tool or replace it ASAP.
Why are you always insisting the tool is broken?
Nxstyle has issues, and it is a little bit like "hare and hedgehog" when a new cornercase pops up, but do you think another tool hasn't to go through improvement cycles?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569#issuecomment-599245358
I didn't address some of these
> The tool itself is supposed to traverse the folder structure down to the file under test, looking for .nxignore files, with inheritance?
That would be a nice design. Beware, however, that this needs to run in both Windows and Unix-like native modes. Makefile.host defines -DCONFIG_WINDOWS_NATIVE=y on the command like in the Windows native case. The primary thing is that you need to select the correct path segment delimitor, '\' or '/', depending on that setting.
Functions like basename() should work without modification so traversing up from the file under test upward would be more portable.
>The top level directory is specified? Or determined from the location of the tool?
The tool could be run from any directory, so that is not useful and the top level directory is not specified. That however, is not a problem if you traverse from the directory containing the file-under-test upward. The top-level directory should contain a landmark like INVIOLABLES.txt or NOTICE (don't depend on .git being present. .git will not be present if users using their own CM system).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] johannes-nivus commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569#issuecomment-599198501
I added the ignore feature.
It can be activated by using
```
<nxs ignore="1" />
```
in either a block or single line comment.
It can be switch off by
```
<nxs ignore="0" />
```
In general it works like the skip ranges from commandline, but inside the file.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] johannes-nivus commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569#issuecomment-599205791
Again my opinion:
There are other tools that use metadata inside source files, doxygen for example.
Why restrict own tools?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] davids5 commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569#issuecomment-599202513
@johannes-nivus I had consider this as well. But I think t is not a good idea to to add the meta data to the source files.
But short term we could do a few things like
CamelCase - there are some well know term and MMCSDIO names that could be grandfathered in.
list them in Nxstyle.
Use warnings not errors on h files in arch
Really I think we should fix the tool or replace it ASAP.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569#issuecomment-599210625
An option would be add a whitelist outside of the source files. Perhaps a file called .nxignore that is in the same directory as the source file. This file could be read, if it exists, to get the whitelisted names.
A higher level .nxignore, perhaps in tools/, could also be used.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] johannes-nivus edited a comment on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
Posted by GitBox <gi...@apache.org>.
johannes-nivus edited a comment on issue #569: Add possibility to add identifier whitelist to "block comments" of file…
URL: https://github.com/apache/incubator-nuttx/pull/569#issuecomment-599198501
I added the ignore feature.
It can be activated by using
```
<nxs ignore="1" />
```
in either a block or single line comment.
It can be switched off by
```
<nxs ignore="0" />
```
In general it works like the skip ranges from command line, but inside the file.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services