[llvm-commits] CVS: llvm/include/llvm/GlobalAlias.h

[Reid Spencer]

Anton,

Some comments ...

On Wed, 25 Apr 2007 11:42:57 -0500
  Anton Korobeynikov <asl at math.spbu.ru> wrote:
> 
> 
> Changes in directory llvm/include/llvm:
> 
> GlobalAlias.h added (r1.1)
> ---
> Log message:
> 
> Add missed file
> 
> 
> ---
> Diffs of the changes:  (+83 -0)
> 
> GlobalAlias.h |   83 
>++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 83 insertions(+)
> 
> 
> Index: llvm/include/llvm/GlobalAlias.h
> diff -c /dev/null llvm/include/llvm/GlobalAlias.h:1.1
> *** /dev/null	Wed Apr 25 11:42:49 2007
> --- llvm/include/llvm/GlobalAlias.h	Wed Apr 25 11:42:39 
>2007
> ***************

> *** 0 ****
> --- 1,83 ----
> + //===______-- llvm/GlobalAlias.h - GlobalAlias class 
>------------*- C++ -*-===//
> + //
> + //                     The LLVM Compiler 
>Infrastructure
> + //
> + // This file was developed by Anton Korobeynikov and 
>is distributed under
> + // the University of Illinois Open Source License. See 
>LICENSE.TXT for details.
> + //
> + 
>//===----------------------------------------------------------------------===//
> + //
> + // This file contains the declaration of the 
>GlobalAlias class, which
> + // represents a single function or variable alias in 
>the VM.

in the VM?

That might be too specific. How about "in the IR" ?


> + //
> + 
>//===----------------------------------------------------------------------===//
> + 
> + #ifndef LLVM_GLOBAL_ALIAS_H
> + #define LLVM_GLOBAL_ALIAS_H
> + 
> + #include "llvm/GlobalValue.h"
> + 
> + namespace llvm {
> + 
> + class Module;
> + class Constant;
> + class PointerType;
> + template<typename ValueSubClass, typename 
>ItemParentClass>
> +   class SymbolTableListTraits;
> + 

You are missing some significant documentation for the 
GlobalAlias class here. Please add doxygen comments with 
an @brief section and perhaps @see sections for 
GlobalValue, Function and GlobalVariable. Your 
documentation needs to discuss what the GlobalAlias is, 
how its used, why we need it, etc.

> + class GlobalAlias : public GlobalValue {
> +   friend class SymbolTableListTraits<GlobalAlias, 
>Module>;
> +   void operator=(const GlobalAlias &);     // Do not 
>implement
> +   GlobalAlias(const GlobalAlias &);     // Do not 
>implement
> + 
> +   void setParent(Module *parent);
> + 
> +   GlobalAlias *Prev, *Next;

Most other LLVM classes put the data members at either the 
top or the bottom of the class declaration. Putting them 
in the middle makes them hard to find.

> +   void setNext(GlobalAlias *N) { Next = N; }
> +   void setPrev(GlobalAlias *N) { Prev = N; }
> + 
> +   const GlobalValue* Aliasee;
> +   std::string Target;
> + public:
> +   /// GlobalAlias ctor - If a parent module is 
>specified, the alias is
> +   /// automatically inserted into the end of the 
>specified modules alias list.

modules -> Module's

> +   GlobalAlias(const Type *Ty, LinkageTypes Linkage, 
>const std::string &Name = "",
> +               const GlobalValue* Aliasee = 0, Module 
>*Parent = 0);
> + 
> +   /// isDeclaration - Is this global variable lacking 
>an initializer?  If so, 
> +   /// the global variable is defined in some other 
>translation unit, and is thus
> +   /// only a declaration here.
> +   virtual bool isDeclaration() const;
> + 
> +   /// removeFromParent - This method unlinks 'this' 
>from the containing module,
> +   /// but does not delete it.
> +   ///
> +   virtual void removeFromParent();
> + 
> +   /// eraseFromParent - This method unlinks 'this' 
>from the containing module
> +   /// and deletes it.
> +   ///
> +   virtual void eraseFromParent();
> + 
> +   virtual void print(std::ostream &OS) const;
> +   void print(std::ostream *OS) const { if (OS) 
>print(*OS); }
> + 
> +   void setAliasee(const GlobalValue* GV);
> +   const GlobalValue* getAliasee() const { return 
>Aliasee; }
> + 
> +   // Methods for support type inquiry through isa, 
>cast, and dyn_cast:
> +   static inline bool classof(const GlobalAlias *) { 
>return true; }
> +   static inline bool classof(const Value *V) {
> +     return V->getValueID() == Value::GlobalAliasVal;
> +   }
> + private:

If you're going to have a private section down here, why 
not put all the private stuff down here? Its more friendly 
to readers of the .h file to list public things first and 
then protected/private.

> +   // getNext/Prev - Return the next or previous alias 
>in the list.
> +         GlobalAlias *getNext()       { return Next; }
> +   const GlobalAlias *getNext() const { return Next; }
> +         GlobalAlias *getPrev()       { return Prev; }
> +   const GlobalAlias *getPrev() const { return Prev; }
> + };
> + 
> + } // End llvm namespace
> + 
> + #endif
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits