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

X3 examples sio fixes #334

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

bonastos
Copy link

Hi JOel, Ichoose to fllow the calc7 example of returning a copy.

@djowel
Copy link
Collaborator

djowel commented Dec 17, 2017

Looks good to me.

@djowel
Copy link
Collaborator

djowel commented Dec 17, 2017

Hmmm, I don't quite like init_helper. Doesn't this fall into the same trap as before? What if a copy of expression is called before the TU constructs init_helper? I think it's better to have add_keywords a static variable in expression().

@Kojoley
Copy link
Collaborator

Kojoley commented Dec 17, 2017

Last time I tried to build x3 examples, calcs were failing to link.

P.S.: CI is not configured to build examples yet. I have not fixed them all so I should probably push the fixes I have, setup Circle-CI with building examples and disable failing ones.

@djowel
Copy link
Collaborator

djowel commented Dec 17, 2017

Also, please be consistent with the coding style: open brace '{' in a new line.

@bonastos
Copy link
Author

It's true, when expression gets called brfore the static ctors, you've got a problem. But the point Iwanted to make on the mailing list was that calling add_keywords() before the TU is initialized invokes member finctions on uninitialized symbol table objects. This is the lesser evil,
To make a separately compiled rule callable before the TU is constructed would (IMHO) mean to construct all objects as part of a meyers singleton.

@djowel
Copy link
Collaborator

djowel commented Dec 18, 2017

Well then in that case, it's better to have expression() return a reference, like before. As long as you don't do anything with expression before main, you should be good. And no one should be doing that anyway.

@bonastos
Copy link
Author

I suspect we're thinking along the same lines, but have a communication problem ...

Wether expression() returns a reference to a Meyers singleton or a copy is an implementation detail. What matters is that the caller receives a fully initialized object.

The problem I see are the other file local objects in expression.o .

Let's compare the current approach and my implentation if statement.o gets initialized before expression.o.

Current approach

  • statement.o: calls expression()
    • calls add_keywords(); undefined behaviour, symboltables in expression.o not initialized yet.
  • expression.o: calls ctors of symbol tables. initialze to empty?
  • main(): will eventually call parse on expression_rule object. All bets are off due to the undefined behaviour

My approach

  • statement.o: calls expression()
  • expression.o:
    • calls ctors of symbol tables.
    • adds keywords in ctor of initialization object
  • main(): will eventually call parse on expression_rule object.

This is why I think that the keywords must be added by a file static ctor. (And yes, I agree that it's not pretty)

As you mentioned, one must not use the expression object before main. But that's true for both approaches.

@djowel
Copy link
Collaborator

djowel commented Dec 18, 2017

It seems we're having a communication problem alright. I agree with your fix with the keywords added by a file static ctor. But you should also return expression by reference so that the reference will track the changes to the rules. If you do a copy, it will return a valid object, but possibly with an empty symbol table.

@bonastos
Copy link
Author

I guess my (admittedly limited) understanding of x3 might be wrong.

So please allow me one last question for my education.

  • The rule object returned from expression() is distinct from the symbol table objects.
  • The only state the rule object carries is the pointer to it's name.
  • It's type is used in statement.o to select the correct parse_rule<>() instantiation.
  • parse_rule<>() in expression.o references the local symbol table objects.
  • by the tone the rule gets called under main() the symbol tables contain the correct values

What difference does it make if expression() returns a copy or a reference?

I don't mind changing the return values to reference, but I'd like to understand the reason why it's necessary.

@djowel
Copy link
Collaborator

djowel commented Dec 19, 2017

Man! You are absolutely correct. I'm the one confused. Haha! I confused QI with X3. Yes. X3 rules are lightweight placeholders and it's the parse_rule that joins the rule placeholders with their definitions. Thanks for setting me straight. Hah! How can I get confused with the libraries that I wrote? :-P

This one is good to go. Pardon the confusion!

@bonastos
Copy link
Author

No problem. I'm just glad that my understanding goes in the right direction :-)

@djowel
Copy link
Collaborator

djowel commented Dec 19, 2017

Now I wonder about this CI thing saying "Some checks haven’t completed yet"... "1 errored and 1 pending checks". @Kojoley ?

@Kojoley
Copy link
Collaborator

Kojoley commented Dec 19, 2017

I wrote above that CI is not run examples, so I have cancelled the jobs to not use the limited CI capacity (apparently, it looks like Appveor did not signal GitHub that the job was cancelled).

@djowel
Copy link
Collaborator

djowel commented Dec 19, 2017

OK, I guess this is good to go then.

@Kojoley
Copy link
Collaborator

Kojoley commented Dec 20, 2017

Squash all the commits.

@djowel
Copy link
Collaborator

djowel commented Dec 20, 2017

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants