r176333 - Add one more sanity check in SourceManager::getFileIDLoaded().

[Argyrios Kyrtzidis]

On Mar 2, 2013, at 11:18 AM, Rafael Espíndola <rafael.espindola at gmail.com> wrote:

>> It's not intentional - if it was we wouldn't have the assert there &
>> this would just be an intended/valid codepath.
>> 
>> If we have some way to take "we're in a state we don't expect to be
>> in" (& importantly: haven't tested) & turn that in to a well defined
>> crash/error reporting that seems to be the right thing to have here.
> 
> Exactly. It gives the developers of the program using libclang the
> opportunity to report you a bug with hopefully enough information to
> fix it. This is similar to chromium's use of breakpad + the CHECK
> macro (http://src.chromium.org/viewvc/chrome/trunk/src/base/location.h).
> 
> When in function f you do
> 
> if (!foo) {
>  assert(0 && ...);
>  return ...;
> }
> 
> You enter a slippery slope. The code calling this function will be in
> an unexpected state and might itself crash, just in a harder to debug
> way. This can lead to

Why do you think that for this specific function, returning FileID() is valid for a return value. If a caller misbehaves because of it, then the caller should be fixed.

> 
> f();
> if (did_f_ignore_a_bug) {
>   // ignore it too.
>   assert(0 && ...);
>   return;
> }
> 
> Note also that this change is in SourceManager.cpp. Could you at least
> move it back in the stack to the caller in libclang?

Not sure how this is possible, do you have specific source changes in mind ?

> 
> Cheers,
> Rafael