Page MenuHomePhabricator

[clang] NFC, move the utility function CompilerInvocation::setLangDefaults to LangOptions.h
ClosedPublic

Authored by hokein on Mar 10 2022, 5:41 AM.

Details

Summary

The function will be moved from clangFrontend to clangBasic, which
allows tools (clang pseudoparser)which don't depend on clangFrontend to use
this function.

Diff Detail

Event Timeline

hokein created this revision.Mar 10 2022, 5:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2022, 5:41 AM
Herald added a subscriber: dexonsmith. · View Herald Transcript
hokein requested review of this revision.Mar 10 2022, 5:41 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: sstefan1. · View Herald Transcript
hokein added a comment.Apr 1 2022, 5:05 AM

in case you missed that, friendly ping :)

dexonsmith requested changes to this revision.Apr 4 2022, 4:31 PM

This makes sense to me! A few comments inline.

Also, the description doesn't talk about timeline for removing the wrapper. Ideally we wouldn't leave behind the wrapper behind... just long enough that you can migrate the callers and delete the old function in separate incremental commit(s) (or if there are very few callers, all in this commit would be fine, but I'm guessing that's not the case here). Or were you thinking something else?

clang/include/clang/Basic/LangOptions.h
561

Looks like a copy/paste typo in the first word.

569–571

I think this would be cleaner as:

class LangOpts {
// ...
  void setDefaults(Language Lang, const llvm::Triple &T, ...);
};

Or setLangDefaults or setDefaultsFor (I don't care about the name, just feel like it makes more sense as a member function if we're updating all the callers anyway).

Also, you should include a default for LangStd or it'll be hard to migrate over callers.

clang/include/clang/Frontend/CompilerInvocation.h
221–222

I think you should inline the new one-liner implementation into the header, documenting what people should do instead.

221–222

I suggest having this comment phrased to explain what it does before saying what people should do. Something like:

/// Forwards to [...].
///
/// TODO: Remove this wrapper once all callers have been updated.
This revision now requires changes to proceed.Apr 4 2022, 4:31 PM

+1 to Duncan's comments, and a couple of nits while here.
Otherwise LG, will be nice to use this without pulling in the grab-bag that is Frontend.

Also, the description doesn't talk about timeline for removing the wrapper. Ideally we wouldn't leave behind the wrapper behind... just long enough that you can migrate the callers and delete the old function in separate incremental commit(s) (or if there are very few callers, all in this commit would be fine, but I'm guessing that's not the case here). Or were you thinking something else?

I only see one usage in-tree (and one more in clspv). And migration is very easy. I think you should do it all in this commit.

clang/include/clang/Basic/LangOptions.h
567

while here: this param is non-obvious and this comment doesn't clarify much.
Maybe "If the language requires extra headers to be implicitly included, they will be appended to this list"

569–571

I kind of like the idea that this logic is "layered above" the langopts struct itself. On the other hand making it a member makes it more discoverable and less surprising that LangOptions is actually an inout param (e.g. IncludeDefaultHeader). Either way is fine with me.

clang/lib/Basic/LangOptions.cpp
223

Pull this out into a separate function getDefaultLangStandard(Language, const Triple&)? (No need to expose it unless you want to, though I think it'll be helpful in future).

It seems bizarre that this depends on the triple, but obviously don't want to change that now

I only see one usage in-tree (and one more in clspv). And migration is very easy. I think you should do it all in this commit.

Nice, agreed, no need to split up.

clang/include/clang/Basic/LangOptions.h
569–571

I kind of like the idea that this logic is "layered above" the langopts struct itself.

@sammccall, I'm curious if you have reasoning for the preference to layer it above; is it because it takes the Triple, or is it something more general? (If it's because of the triple, I agree that makes the layering a bit odd.)

On the other hand making it a member makes it more discoverable and less surprising that LangOptions is actually an inout param (e.g. IncludeDefaultHeader).

Maybe it's better to return by value in either case to remove the inout, since it seems unnecessary:

class LangOpts {
// ...
  static LangOpts getDefaults(Language Lang, const llvm::Triple &T, ...);
};

If you still prefer a free function, I'd be happy enough with something like this:

namespace clang {
LangOpts getLangDefaults(Language Lang, const llvm::Triple &T, ...);
}

(I'm probably almost indifferent at this point, after thinking about the triple, ...)

sammccall added inline comments.Apr 6 2022, 3:00 AM
clang/include/clang/Basic/LangOptions.h
569–571

@sammccall, I'm curious if you have reasoning for the preference to layer it above; is it because it takes the Triple, or is it something more general?

It's more about compiler defaults being an application-level concern where LangOptions is more of a dumb struct. But that's also an argument for keeping it in Frontend, and we don't want that for practical reasons (it's nice to use the lexer on real code without Frontend!). So I'm not sure I have a coherent argument here, I'm happy with any option.

Return by value sounds great, unfortunately the existing code in Frontend calls this in the *middle* of initializing LangOpts from various sources, so it would imply some bigger/riskier changes I guess.

dexonsmith added inline comments.Apr 6 2022, 12:13 PM
clang/include/clang/Basic/LangOptions.h
569–571

Return by value sounds great, unfortunately the existing code in Frontend calls this in the *middle* of initializing LangOpts from various sources, so it would imply some bigger/riskier changes I guess.

Looking more closely, you're right that initialization is pretty twisty; I don't think it's worth the risk for now.

In which case, I like the member function approach, even though it makes LangOpts a little less dumb. Something like LangOpts::setLangDefaults(), I guess. @hokein, if you'd strongly prefer a free function (what you already have) I'd be fine with that too.

hokein updated this revision to Diff 421494.Apr 8 2022, 5:17 AM
hokein marked 2 inline comments as done.

address comments;
move to LangOpts::setLangDefaults;
pull out a getNameKind in LangStandard;
remove the old API in CompilerInvocation;

hokein added a comment.Apr 8 2022, 5:17 AM

Thanks for all comments!

clang/include/clang/Basic/LangOptions.h
569–571

Personally, I don't have a strong opinion, I'm fine with either. Change to a method of LangOpts.

clang/lib/Basic/LangOptions.cpp
223

I think this is a good idea, there is a similar getLangKind method in LangStandard. Moved it to LangStandard.

sammccall accepted this revision.Apr 11 2022, 8:04 AM
sammccall added inline comments.
clang/include/clang/Basic/LangStandard.h
141

this makes it sound like a simple getter/lookup, which it absolutely is not!

getDefaultLanguageStandard? And separate it out from the simple getters?

(The meaningless distinction in the other function names between "standard" and "kind", and the "lang" abbreviation are both pretty unfortunate here, we don't need to fix them but let's not propagate them further)

dexonsmith accepted this revision.Apr 11 2022, 10:23 AM

LGTM once @sammccall 's new comments are addressed!

This revision is now accepted and ready to land.Apr 11 2022, 10:23 AM
hokein updated this revision to Diff 422412.Apr 13 2022, 12:23 AM
hokein marked an inline comment as done.

getDefaultLanguageStandard.

This revision was landed with ongoing or failed builds.Apr 13 2022, 12:53 AM
This revision was automatically updated to reflect the committed changes.