[cfe-commits] r44942 - /cfe/trunk/include/clang/Basic/SourceLocation.h

[Ted Kremenek]

On Dec 12, 2007, at 10:43 AM, Chris Lattner wrote:

>> URL: http://llvm.org/viewvc/llvm-project?rev=44942&view=rev
>> Log:
>> Added class FullContextSourceLocation: a tuple class that
>> contains both a SourceLocation and its associated
>> SourceManager. This class is useful for argument passing to
>> functions that expect both objects.
>
> Hey Ted,
>
>
>> +/// FullContextSourceLocation - A tuple containing both a  
>> SourceLocation
>> +///  and its associated SourceManager.  Useful for argument  
>> passing to functions
>> +///  that expect both objects.
>> +class FullContextSourceLocation {
>> +  SourceLocation Loc;
>> +  SourceManager* SrcMgr;
>> +public:
>
> Ok.
>
>> +  explicit FullContextSourceLocation(SourceLocation loc)
>> +    : Loc(loc), SrcMgr(NULL) {}
>
> I don't think we want to support fullsourceloc's with a null srcmgr  
> unless the location is invalid.  It seems best to support two ctors:  
> a default ctor that makes an invalid sloc, and a version that takes  
> both.
>
>> +
>> +  explicit FullContextSourceLocation(SourceLocation loc,  
>> SourceManager& smgr)
>> +    : Loc(loc), SrcMgr(&smgr) {}
>
> I think all clients that decode the sloc only need read-only access  
> to the source manager.  I'd suggest making all references to it const.
>
>> +  SourceLocation getSourceLocation() const { return Loc; }
>> +  operator SourceLocation() const { return Loc; }
>
> Ick.  Please name this getLoc(), and remove the implicit conversion :)
>
>> +
>> +  SourceManager& getSourceManager() {
>
> Please drop this one (no need for a non-const one).
>
>> +  const SourceManager& getSourceManager() const {
>
> How about getManager() ?
>
> -Chris

Agreed on all counts.