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

Output declarations in sorted order by name. #27

Merged
merged 1 commit into from
Apr 4, 2017

Conversation

pwaller
Copy link
Contributor

@pwaller pwaller commented Apr 4, 2017

Using a specially written tool for the problem, I inspected all loops over maps (all 6 of them), and spotted the one that was causing a problem.

The file output is still not deterministic because of the timestamp in the file.

May I suggest removing the timestamp? Then the output will be fully determinstic and diff can be used to determine file equality.

@xlab
Copy link
Owner

xlab commented Apr 4, 2017

Thank you very much for finding the root cause! And for the tool too, I have a few other project with the same issue..

As for timestamps, I won't remove them, as I want the header to reflect the changes in files, so it also could be seen in diffs and commits. It's like a seal of freshness. But I can add a flag for your convenience.

@xlab xlab merged commit fb37709 into xlab:master Apr 4, 2017
@xlab
Copy link
Owner

xlab commented Apr 4, 2017

@pwaller
I just added -nostamp for your cases. Good luck!

@pwaller pwaller deleted the make-order-deterministic branch April 5, 2017 07:51
@pwaller
Copy link
Contributor Author

pwaller commented Apr 5, 2017

In any case I would not use a -nostamp option - it's more configuration, more ways that I can make projects using c-for-go different from one another. You might want to consider removing that, since it's more options and complexity than are needed. Also -nofoo means you're logically saying (!foo) == true, which is a classic thing that can lead to confusing logic like !(!(foo) == true). If you really want to keep the option for your own use (again, I'm not going to use it) - you could call it -stamp and default it to true, that way you don't need a DisableTimestamps function.

I would also further discuss putting the timestamp in the file itself:

  • The presence of a -nostamp option does not fix the problem of me regenerating bindings that live in your repositories (generated without -nostamp) and having the resulting files change.

  • The timestamps are visible in git log and ls -l or if you want to see the full precision with ls you can use ls -l --time-style=full-iso.

Put another way: Why duplicate the timestamp information, and add more configuration knobs, complicate version control and testing when there is already exists a standard way to see the age of files which work on any type of file?

One last thing, there is a standard format for the "this code is autogenerated" comment that you might want to use. It means that tooling can detect the auto-generated code automatically.

@xlab
Copy link
Owner

xlab commented Apr 5, 2017

@pwaller Thanks for your feedback on these issues.

I'm not going to dive into debates how to name the flag when it covers 1 case out of 100, overriding the standard behaviour of writing a timestamp there. Neither would I consider any formatting options that looks like a typical bike sched going to be ignited there.

There are other options as well, you can skip first lines when making a diff, e.g. tail -n +2 would resolve the cause as well, if you don't like the flag.

One last thing, there is a standard format for the "this code is autogenerated" comment

Good to know! I will wait for more examples what exactly happens when tooling detects autogenerated files, because the feature is relatively new and the most use cases I could find is to ignore these files. I don't want the tooling ignore these files automatically.

xlab added a commit that referenced this pull request Apr 17, 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.

None yet

2 participants