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.

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

Parsetools enhancement #2589

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

crownedgrouse
Copy link
Contributor

Optimizations:

  • yecc : merge two subsequents tests into one
  • leex : compile regex once
    New feature:
  • leex : allow external definition inclusion
    Fix :
  • Silent a warning

	This permit same result when using a sys.config file
        in a release or starting manually a node with -config ...
	For backward compatibility, if file is not found relative
        to sys.config, it is searched from working directory like before.

	Fix after first review

	- rephrase documentation
	- remove check_conf_sys/3
	- remove quote on atom
	- fold comment
	Optimizations:
	- yecc : merge two subsequents tests into one
	- leex : compile regex once
        New feature:
	- leex : allow external definition inclusion
        Fix :
	- Silent a warning
@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Apr 5, 2020
@rickard-green
Copy link
Contributor

@crownedgrouse How come so many PRs? You can modify a PR if needed.

@crownedgrouse
Copy link
Contributor Author

last one, sorry.

@uabboli
Copy link
Contributor

uabboli commented Aug 27, 2021

Hi,

Sorry for the very long delay. I couldn't make up my mind about this
feature when the PR arrived, and I'm still not convinced there is a
use case for it. I think it's best for someone else to decide.

A few remarks, though:

IMHO, the modification in yecc.erl is unnecessary.

I think you can remove the line starting with _Filename in leex_SUITE
completely.

leex.erl already has superfluous spaces, but I think you should try
not to introduce new ones. The ones I can see in a quick glance are
at the end of {ok,Incfile} in parse_defs().

You should have at least one test case where the name of the included
file has characters greater than 255. Things I haven't checked are:
Is "/" allowed in filenames?
Do Windows filenames work as they should ("C:", "\")?

There should be test cases where the .xrl files have Latin-1 encoding
and UTF-8 encoding respectively, and they include a file with the same
encoding.

It is possible to give an encoding at the beginning of .xrl files.
Maybe it should be possible to state the encoding at the beginning of
included files as well, to be consistent. Test cases are needed for
that, of course.

What happens if a file includes itself? There are no warnings, it
seems.

There are few minor things in the docs that need to be fixed. Here is
a suggestion:

<p>Definitions are sometimes depending on OS, architecture, locale or 
  something else. In order to avoid to have as many <c>.xrl</c> files as 
  different cases, definitions can be pre-processed and written in a
  file referenced in the <c>Definitions.</c> section.
</p>

I removed the next paragraph as I don't think it's needed (and I
didn't quite get it).

<code>
  Definitions.
  include "external.inc"
  Rules.
  [...]      
</code>
<note><p>Included files can contain definitions only, 
without the <c>Definitions.</c> section. Several included files can be used, 
optionally mixed with the usual definitions in the <c>.xrl</c> file.</p></note>

Best regards,
Hans Bolinder, Erlang/OTP team, Ericsson

@uabboli uabboli removed their assignment Aug 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:VM Assigned to OTP team VM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants