[cfe-commits] r144269 - in /cfe/trunk: include/clang-c/ include/clang/Frontend/ lib/Frontend/ tools/c-index-test/ tools/libclang/

[Ted Kremenek]

On Nov 10, 2011, at 10:32 AM, Argyrios Kyrtzidis wrote:

>> +void CXLoadedDiagnostic::decodeLocation(CXSourceLocation location,
>> +                                        CXFile *file,
>> +                                        unsigned int *line,
>> +                                        unsigned int *column,
>> +                                        unsigned int *offset) {
>> +  
>> +  
>> +  // CXSourceLocation consists of the following fields:
>> +  //
>> +  //   void *ptr_data[2];
>> +  //   unsigned int_data;
>> +  //
>> +  // The lowest bit of ptr_data[0] is always set to 1 to indicate this
>> +  // is a persistent diagnostic.
>> +  //
>> +  // For now, do the unoptimized approach and store the data in a side
>> +  // data structure.  We can optimize this case later.
>> +  
>> +  uintptr_t V = (uintptr_t) location.ptr_data[0];
>> +  assert((V & 0x1) == 1);
>> +  V &= ~(uintptr_t)1;
>> +  
>> +  const Location &Loc = *((Location*)V);
>> +  
>> +  if (file)
>> +    *file = Loc.file;  
>> +  if (line)
>> +    *line = Loc.line;
>> +  if (column)
>> +    *column = Loc.column;
>> +  if (offset)
>> +    *offset = Loc.offset;
>> +}
> 
> Is this modification to CXSourceLocation so that the diagnostic location becomes independent of a SourceManager ?

Yes.  The idea is that deserialized diagnostics don't have a source location.  They don't actually need one since all this locations are raw physical locations anyway.

> 
> Why not introduce a new kind of location (like CXFileLocation or something) instead ?

I couldn't disagree with you more.  Requiring clients to use a different set of mostly duplicate APIs for source locations which are just different under the hood seems like extremely poor API design to me.  It makes the API verbose for no reason at all.  Why should the client be concerned at all about how the CXSourceLocation is implemented under the hood as long as it provides the same functionality?

Keep in mind this would also require duplicating a bunch of the CXDiagnostics API to distinguish between diagnostics that have CXFileLocations and those that have CXSourceLocations.

> If you remove the SourceManager you "disable" the functionality of CXSourceLocation anyway, e.g. while previously you could go through the expansion stack, get presumed location, etc., you can't do that with this new kind of location.

Sorry, but I think that statement is not true.  The prime use of CXSourceLocations is to query file, line, and column information.  That is still supported.

We aren't disabling any functionality.  Yes, we are losing the macro expansion trace and the inclusion stack, but the locations are actual locations.  They are just the spelling locations.  In the absence of macro expansions, these are the same exact locations that a client would see in the non-deserialized diagnostics case.  One way to think of it is that the locations have been "pre-lowered" for consumption by a client that just wants to issue warnings at actual lines/columns in code.  We can still include all the macro expansion goop, but pre-lower those as parts of the diagnostic instead of encoding them into the CXSourceLocations.

> Seems better to be clear in the API.

I think duplicating most (all) of the same CXSourceLocation APIs just to expose a detail that clients should not be thinking about anyway adds no clarity to the API.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111110/dba4a7e8/attachment.html>