sepavloff marked 3 inline comments as done.
sepavloff added a comment.
In https://reviews.llvm.org/D40508#936617, @rnk wrote:
> It's not clear to me that this abbreviation functionality should live in
> Support. Clang probably wants enough control over this (assuming we're doing
> it) that the logic should live in clang.
>
> I also think we might want to try solving this a completely different way:
> just don't bother emitting more than two template arguments for IR type
> names. We don't really need to worry about type name uniqueness or matching
> them across TUs.
Actually the intention is to have unique type names across different TUs.
I will publish the relevant patch a bit latter, but the problem we are solving
is in incorrect behavior of `llvm-link`. If one TU contains opaque type and the
other has the type definition, these two types must be merged into one. The
merge procedure relies on type names, so it is important to have the same type
names for types in different TUs that are equivalent in the sense of C++.
================
Comment at: include/clang/AST/PrettyPrinter.h:231
+ /// make things like breaking digraphs and '>>' in template parameters.
+ bool NotForCompilation : 1;
};
----------------
rjmccall wrote:
> This saves, what, a few spaces from a few thousand IR type names? I'm
> skeptical that this even offsets the code-size impact of adding this option.
Not these few spaces themselves make the issue. The real evil is that to insert
these spaces, `printTemplateArgumentList` had to print each template parameter
into intermediate stream. We could try to use `raw_abbrev_ostream` to reduce
memory consumption, it would not work because these intermediate streams are of
type `raw_svector_ostream` and all these huge parameter texts would be
materialized first and only then would go to compacting stream.
If no need to maintain compilable output, intermediate streams are not needed
and all input can go directly to `raw_abbrev_ostream`.
================
Comment at: lib/AST/TypePrinter.cpp:1532-1534
+namespace {
+template<typename TA>
+void printTo(raw_ostream &OS, ArrayRef<TA> Args, const PrintingPolicy &Policy,
----------------
rnk wrote:
> `static` is nicer than a short anonymous namespace.
Yes, but this is function template. It won't create symbol in object file.
Actually anonymous namespace has no effect here, it is only a documentation
hint.
https://reviews.llvm.org/D40508
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
