[cfe-commits] r44266 - in /cfe/trunk: AST/Stmt.cpp AST/StmtPrinter.cpp AST/StmtSerialization.cpp Parse/ParseStmt.cpp Sema/Sema.h Sema/SemaStmt.cpp include/clang/AST/Stmt.h include/clang/Parse/Action.h include/clang/Parse/Parser.h
Ted Kremenek
kremenek at apple.com
Wed Nov 21 23:16:45 PST 2007
Anders,
The serialization code looks good to me. A few comments (inline):
On Nov 21, 2007, at 5:36 PM, Anders Carlsson wrote:
> Modified: cfe/trunk/AST/StmtSerialization.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/AST/StmtSerialization.cpp?rev=44266&r1=44265&r2=44266&view=diff
>
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- cfe/trunk/AST/StmtSerialization.cpp (original)
> +++ cfe/trunk/AST/StmtSerialization.cpp Wed Nov 21 19:36:19 2007
> @@ -197,8 +197,27 @@
>
> void AsmStmt::EmitImpl(Serializer& S) const {
> S.Emit(AsmLoc);
> +
> getAsmString()->EmitImpl(S);
Directly emitting the StringLiteral object is okay if you believe that
no object outside of the AsmStmt will EVER hold a reference to it.
Otherwise, I would use EmitOwnedPtr. This reasoning applies to all
the places where StringLiteral appears in AsmStmt. If you don't use
EmitOwnedPtr (since I assume AsmStmt owns the references to these
StringLiterals), the serializer won't be able to backpatch or
otherwise fix up other references to these objects upon deserialization.
>
> S.Emit(RParenLoc);
Eventually we will probably reorganize the order of how some of these
fields are emitted because it will result in a smaller size on disk.
For example, the SourceLocation objects and integers (e.g., the sizes)
should probably be grouped together in the front, as the Serializer
can group them nicely into a single record, while the StringLiteral
object and the strings will likely terminate a record. This isn't so
important now; once we start adding support for abbreviations in the
Serializer than we can play all sorts of tricks to reducing the size.
AsmStmts are also not so common as other statements, so it isn't as
big a deal to squeeze out every bit we can from the pickled format.
The important issue now is that the implementation is correct, which
it appears to be from looking at the code. If you can continue to
add tests cases into the clang test suite that tests the functionality
of AsmStmt, then we will be able to readily test that this
serialization code is correct using the --test-pickling in the driver.
<SNIP>
>
> AsmStmt* AsmStmt::CreateImpl(Deserializer& D) {
> @@ -206,7 +225,37 @@
> StringLiteral *AsmStr = StringLiteral::CreateImpl(D);
> SourceLocation PLoc = SourceLocation::ReadVal(D);
<SNIP>
>
> + Stmt->Names.reserve(size);
> + for (unsigned i = 0; i < size; ++i) {
> + std::vector<char> data;
One recommendation is to move the vector "data" outside of the loop,
and invoke "data.clear()" at the beginning of the loop body. This
will reduce the number of allocations made when the vector is resized.
Looks good! I'm glad you could successfully de-obfuscate my
completely undocumented serialization API.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20071121/456d06bd/attachment.html>
More information about the cfe-commits
mailing list