[cfe-commits] r44631 - in /cfe/trunk/Driver: TranslationUnit.cpp TranslationUnit.h

[Chris Lattner]

On Dec 5, 2007, at 1:36 PM, Ted Kremenek wrote:
> Author: kremenek
> Date: Wed Dec  5 15:36:08 2007
> New Revision: 44631
>
> URL: http://llvm.org/viewvc/llvm-project?rev=44631&view=rev
> Log:
> Added "TranslationUnit" class that will be used to provide an  
> interface
> for serializing/deserializing ASTs that is decoupled from the logic
> in SerializationTest (which will soon be rewritten to use this  
> interface).

Hi Ted,

> +++ cfe/trunk/Driver/TranslationUnit.cpp Wed Dec  5 15:36:08 2007
> @@ -0,0 +1,229 @@
> +//===--- TranslationUnit.cpp - Abstraccction for Translation Units  
> --------===//

You're stuttering :)

>
> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file was developed by Ted Kremenek and is distributed under
> +// the University of Illinois Open Source License. See LICENSE.TXT  
> for details.
> +//

You're missing a divider here.

>
> +// FIXME: This should eventually be moved out of the driver, or  
> replaced
> +//        with its eventual successor.

I think this should move to libast?

>
> +//
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +
> +#include "TranslationUnit.h"
> +#include "clang.h"
> +
> +#include "clang/Basic/TargetInfo.h"
> +#include "clang/Basic/SourceManager.h"
> +#include "clang/AST/AST.h"
> +
> +#include "llvm/Bitcode/Serialize.h"
> +#include "llvm/Bitcode/Deserialize.h"
> +#include "llvm/Support/MemoryBuffer.h"
> +#include "llvm/System/Path.h"
> +#include "llvm/ADT/scoped_ptr.h"
> +
> +#include <stdio.h>

Please use <cstdio>

>
> +
> +namespace {
> +  enum { BasicMetadataBlock = 1,
> +         ASTContextBlock = 2,
> +         DeclsBlock = 3 };
> +}
> +
> +using namespace clang;
> +
> +bool TranslationUnit::EmitBitcodeFile(llvm::sys::Path& Filename)  
> const {

I don't think this should be a method on TranslationUnit.  This should  
be a global function somewhere that takes a translation unit as an  
argument.  An analog is the LLVM WriteBitcodeToFile function.  Please  
split this out of TranslationUnit.cpp.  Likewise for ReadBitcodeFile.

> +void TranslationUnit::Emit(llvm::Serializer& Sezr) const {
> +
> +  // ===---------------------------------------------------===/
> +  //      Serialize the top-level decls.
> +  // ===---------------------------------------------------===/
> +
> +  Sezr.EnterBlock(DeclsBlock);
> +
> +  // Only serialize the head of a decl chain.  The ASTConsumer  
> interfaces
> +  // provides us with each top-level decl, including those nested in
> +  // a decl chain, so we may be passed decls that are already  
> serialized.
> +  for (const_iterator I=begin(), E=end(); I!=E; ++I)
> +      if (!Sezr.isRegistered(*I))
> +        Sezr.EmitOwnedPtr(*I);

Funky indentation.

>
> +
> +  Sezr.ExitBlock();
> +
> +  // ===---------------------------------------------------===/
> +  //      Serialize the "Translation Unit" metadata.
> +  // ===---------------------------------------------------===/

How about:

> +  //===---------------------------------------------------===//


Takes the same space and is more consistent :)

> +++ cfe/trunk/Driver/TranslationUnit.h Wed Dec  5 15:36:08 2007
> @@ -0,0 +1,75 @@
> +//===--- TranslationUnit.h - Abstraction for Translation Units   
> -----------===//
> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file was developed by Ted Kremenek and is distributed under
> +// the University of Illinois Open Source License. See LICENSE.TXT  
> for details.
> +//

Needs divider here, should also go into AST probably.

>
> +// FIXME: This should eventually be moved out of the driver, or  
> replaced
> +//        with its eventual successor.
> +//
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +
> +#ifndef LLVM_CLANG_TRANSLATION_UNIT_H
> +#define LLVM_CLANG_TRANSLATION_UNIT_H
> +
> +#include "clang/Basic/LangOptions.h"
> +#include "llvm/Bitcode/SerializationFwd.h"
> +#include "llvm/System/Path.h"

Please forward declare sys::Path instead of #including it.

+class TranslationUnit {
> +  LangOptions LangOpts;
> +  ASTContext* Context;

Can this embed the ASTContext by-value like LangOpts?

>
> +  std::list<Decl*> TopLevelDecls;

Please use an std::vector, std::list is really inefficient for this.

-Chris