Skip to content

Conversation

@sdkrystian
Copy link
Member

No description provided.

@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://784.mrdocs.prtest2.cppalliance.org/index.html

@alandefreitas
Copy link
Collaborator

So cmdLineInputs has priority over --config?

@sdkrystian
Copy link
Member Author

@alandefreitas yes... there is a way to do this in llvm itself (I think), but that requires adding features to the configuration generator

@alandefreitas
Copy link
Collaborator

alandefreitas commented Jan 6, 2025

That looks weird to me.

Why would anyone attempt to implicitly set the --config option as part of the positional inputs and explicitly via --config?

In the weird case that the user does that, why would the implicit option have precedence over the explicit option? If someone is set explicitly, it needs to have precedence by definition.

Maybe the commit achieves something else and I'm just missing the point. But if the only point is to make the implicit option have precedence over the explicit one, that looks like it maximizes surprise and achieves nothing in return. I'm having a hard time thinking of an example where this would solve a problem.

@alandefreitas
Copy link
Collaborator

Another point is this behavior would be in conflict with what PublicSettingsVisitor does. Do different parts of the code could assume different config files. Or the effective config file could randomly change.

@sdkrystian
Copy link
Member Author

That looks weird to me.

Why would anyone attempt to implicitly set the --config option as part of the positional inputs and explicitly via --config?

I don't know. I don't think we should accept config files as positional arguments -- only via --config. Nonetheless, we support passing both ways, and that is a problem because we parse the arguments prior to knowing whether a config file was passed as a positional argument (unless we use c_callback or something), resulting in the default config path being used.

@alandefreitas
Copy link
Collaborator

I don't know. I don't think we should accept config files as positional arguments -- only via --config.

Yes. This is for backward compatibility. We don't really need backward compatibility at this point (0.0.3) but I just didn't want people yelling at me at the time.

Nonetheless, we support passing both ways, and that is a problem because we parse the arguments prior to knowing whether a config file was passed as a positional argument (unless we use c_callback or something),

I don't agree with that. The function this PR changes has access to both the inputs (toolArgs.cmdLineInputs) and the positional argument (toolArgs.config).

Maybe you're talking about the normalization logic in Config.cpp. We could see if we have some bug there. But this PR doesn't touch that.

resulting in the default config path being used.

Can you provide an example command line where the current implementation would lead to an incorrect result?

@alandefreitas
Copy link
Collaborator

Once I understood the problem, I believe #785 obsoleted this PR. Please have a look.

@alandefreitas
Copy link
Collaborator

Obsoleted by c73a03b and f3e0b4a

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