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

Rafael EspĂ­ndola rafael.espindola at gmail.com
Sat Mar 2 11:18:59 PST 2013


> 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

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?

Cheers,
Rafael



More information about the cfe-commits mailing list