erichkeane added inline comments.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2048
+ QualType Type) {
+ static llvm::DenseMap<QualType, StringRef> Types;
if (Types.empty()) {
----------------
yihanaa wrote:
> erichkeane wrote:
> > yihanaa wrote:
> > > erichkeane wrote:
> > > > yihanaa wrote:
> > > > > yihanaa wrote:
> > > > > > erichkeane wrote:
> > > > > > > yihanaa wrote:
> > > > > > > > erichkeane wrote:
> > > > > > > > > yihanaa wrote:
> > > > > > > > > > erichkeane wrote:
> > > > > > > > > > > Instead of this initialization, can we make this a
> > > > > > > > > > > constexpr constructor/collection of some sort and
> > > > > > > > > > > instantiate it inline?
> > > > > > > > > > > Instead of this initialization, can we make this a
> > > > > > > > > > > constexpr constructor/collection of some sort and
> > > > > > > > > > > instantiate it inline?
> > > > > > > > > >
> > > > > > > > > > I don't have a good idea because it relies on Context to
> > > > > > > > > > get some types like const char *
> > > > > > > > > We could at minimum do a in inline-initializer instead of the
> > > > > > > > > branch below.
> > > > > > > > I still haven't come up with a good idea to do it๐
> > > > > > > Hrmph, `DenseMap` doesn't seem to have an init list ctor?!
> > > > > > >
> > > > > > > Since this is a finite, known at compile time list of low 'N',
> > > > > > > I'd suggest just making it `static std::array<pair<QualType,
> > > > > > > StringRef>> Types = {{Context.CharTy, "%c"},...`.
> > > > > > >
> > > > > > > Then just doing a llvm::find_if. The list is small enough I
> > > > > > > suspect the DenseMap overhead is more than the use of
> > > > > > > `llvm::find_if`.
> > > > > > Haha,DenseMap also has no constexpr ctor. I think the time
> > > > > > complexity of these two is the same.
> > > > > The Context.CharTy... etc. are not static, So we might need a static
> > > > > value instead of it, and compare it with QualType
> > > > They don't have to be static? This should do 'magic static'
> > > > initialization.
> > > I can't agree more. it might be a type identifier, such as
> > > getAsString()'s return value
> > Actually, I don't think the collection can be static! When we re-use the
> > same process for multiple TUs, we get a new ASTContext. SO this would
> > result in us having types that don't necessarily 'match'. I think this
> > collection/checks need to either be stored in ASTContext, or in an
> > 'if-else' tree here.
> I also don't think it should be maintained in this place. Shouldn't this be
> better maintained together with the printf parameter checking part, what do
> you think?
I'm unfamiliar with what you mean.
HOWEVER, the comments by @rsmith and @rjmccall I think are blocking on this
feature/functionality anyway. So we might still wish to refactor this builtin
significantly based on what they come up with.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122822/new/
https://reviews.llvm.org/D122822
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
