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

Add a file tag parser to core:odin/parser #4009

Merged
merged 7 commits into from
Aug 30, 2024

Conversation

thetarnav
Copy link
Contributor

Having a way to parse +build, +private and other flags will be useful for ols and possibly other meta-programming tools.

I tried to copy the implementation from the cpp parser, but the important difference is that the parser in the compiler parses the tags to match the current target and stop parsing if not matched, when this added parser builds a custom structure to let the user handle the tags as needed. For example in ols we probably want to parse all of the files, but use the build tags to decide when to not show the autocomplete.

Currently the implementation is missing error handling and the +vet tag.

Also it is not integrated into the parse_file proc, and provided as a separate helper.

@thetarnav
Copy link
Contributor Author

Actually I see that parsing project names is incorrect because it doesn't allow for groups foo, bar, although not sure what use do they have, I guess for consistency with the +build tag

@laytan
Copy link
Sponsor Collaborator

laytan commented Aug 28, 2024

This looks like a good addition, we did add riscv64 since this PR so could you do a rebase with master and add it?

Also about your last comment on project names, I honestly have no clue what that even does 😆 , I do think it should be handled the same way as the cpp parser if possible.

@laytan
Copy link
Sponsor Collaborator

laytan commented Aug 28, 2024

Additionally, some of these constants may make a lot of sense in base:runtime.

These 2 bit sets: https://github.com/odin-lang/Odin/pull/4009/files#diff-7f423f935d8ea5cace97cde5364d6c20ac9f5f09680eba4a274891039b0a83f8R17-R18

And these LUTs and ALL_ constants: https://github.com/odin-lang/Odin/pull/4009/files#diff-7f423f935d8ea5cace97cde5364d6c20ac9f5f09680eba4a274891039b0a83f8R34-R81

The LUTs could also be removed entirely and replaced with reflection, but I am not for or against it.

@require_results
get_build_os_from_string :: proc(str: string) -> Odin_OS_Type {
	fields := reflect.enum_fields_zipped(Odin_OS_Type)
	for os in fields {
		if strings.equal_fold(os.name, str) {
			return Odin_OS_Type(os.value)
		}
	}
	return .Unknown
}

@laytan
Copy link
Sponsor Collaborator

laytan commented Aug 28, 2024

although not sure what use do they have, I guess for consistency with the +build tag

I've just been told what they are for and having groups does make sense.

Let's say you have 3 projects foo, bar and baz and have a common package between them to share code, say foobarbaz that package could then use //+build-project-name foo, bar to do specific things for the first 2 projects.

@thetarnav
Copy link
Contributor Author

thetarnav commented Aug 29, 2024

thanks for the review

Also about your last comment on project names, I honestly have no clue what that even does 😆 , I do think it should be handled the same way as the cpp parser if possible.

The way they are being handled in cpp parser is pretty simple
Which is also a problem
The cpp parser just goes through the values until it finds one that matches the current project name, no allocations, no data structures.
Here I wanted to have a parser that returns all of the tags as data.
And some tags need to be dynamically allocated, like the project names.

Also what I meant with my last comment was that the particular syntax of //+build-project-name <group>, <group> where the groups can have multiple names doesn't have that much sense, it must be the way it is for consistency with the +build tag.

For example here, the fist one will match either foo or bar, and the second will match none because a project cannot be named both foo and bar.

//+build-project-name foo, bar
//+build-project-name foo bar

And here there will be no difference between the two:

//+build-project-name !foo, !bar
//+build-project-name !foo !bar

Also combining negative with positive names doesn't have much sense:

//+build-project-name foo !bar

Especially this:

//+build-project-name foo !foo

While all of the above parse with no errors.

So theoretically the parser could still return project names as []string if it was made a bit smarter to flatten the groups.
But it's probably better to provide a separate match_build_tags helper that handles it and make project names [][]string.

The LUTs could also be removed entirely and replaced with reflection

Thanks, I had no idea about this.
Since the parser already depends on reflect indirectly it might not be a bad idea.

@laytan
Copy link
Sponsor Collaborator

laytan commented Aug 29, 2024

Ah I see, thanks for the clarification, the way you do it here looks fine then.

@gingerBill gingerBill merged commit 773703b into odin-lang:master Aug 30, 2024
7 checks passed
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