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

Replace duplicate assembly.S with include #171

Closed
wants to merge 1 commit into from

Conversation

jtraglia
Copy link
Contributor

The contents of this file can be replaced with a single include. I think this is simpler and less error prone.

@dot-asm
Copy link
Collaborator

dot-asm commented Jul 7, 2023

Relative paths in #include-s are a no-go in my book. Because the standard is kind of ambiguous in that respect. Well, it says "implementation dependent" and formally speaking one can make a case that in this particular case the implementation is effectively known. In the sense that cgo makes certain assumptions about the C compiler it calls, and relative paths in #include are among those assumptions... At some point I've considered alternative names for the assembly.S and server.c in the go directory that would include their counterparts through -I${SRCDIR}. I'd still argue that it would be more appropriate. The original reason for why it wasn't done from the get-go was the desire to maintain a degree of freedom in the face of uncertainty. One can argue that we now know enough to realize that there is no uncertainty to "fear" :-)

@dot-asm
Copy link
Collaborator

dot-asm commented Jul 7, 2023

alternative names for the assembly.S and server.c in the go directory

Just in case, yes, both. If you're concerned about rb_tree.c, then one can note that it is not actually called, but if unused code is a concern, it can be #ifndef __BLST_CGO__-ed.

@jtraglia
Copy link
Contributor Author

jtraglia commented Jul 7, 2023

Yes, I like that idea; renaming both files & doing non-relative includes.

@dot-asm dot-asm closed this in 4967efe Jul 9, 2023
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.

2 participants