Page MenuHomePhabricator

Enhance TableGen error message capabilities
ClosedPublic

Authored by Paul-C-Anagnostopoulos on Sep 9 2020, 9:39 AM.

Details

Summary

This is the first step in enhancing TableGen's error message capabilities. I added the source location to the RecordVal class, so that now source locations are associated with both the Record and RecordVal classes. This allows more precise error messages in backends. I added two PrintFatalError methods that accept a Record or a RecordVal.

As a test of this, I improved the error messages in the SearchableTableEmitter backend. Every message now includes at least the record definition location and quite often the field definition location. I also made its messages more consistent.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2020, 9:39 AM
Paul-C-Anagnostopoulos requested review of this revision.Sep 9 2020, 9:39 AM

This patch looks generally good to me (but please fix the clang format issues if you can), but this is detailed enough that it would be best to find another reviewer to give you a full tech review, as I haven't been in this code for (too) many years :)

Thanks! I will definitely fix the formatting issues.

@nhaehnle: Can you review this? Or recommend another reviewer?

I reformatted the code according to the Lint suggestions.

madhur13490 added inline comments.
llvm/lib/TableGen/Record.cpp
2071

I think function can be very well written with early-return pattern i.e. check for the condition, if fails, return false. This way you can avoid nested "if" blocks.

2082

Please capitalize induction variable.

@madhur13490 I simply copied the preceding setValue() function. Let me commit this the way it is and then I will revisit both functions.

I will capitalize the induction variable.

@madhur13490 I simply copied the preceding setValue() function. Let me commit this the way it is and then I will revisit both functions.

I will capitalize the induction variable.

Sure, thanks!

Paul-C-Anagnostopoulos set the repository for this revision to rG LLVM Github Monorepo.

I capitalized the I and E variables in the two SetValue() functions.

I will revisit them in a future pass to reorganize for early returns.

Just a bump to request a full review.

madhur13490 accepted this revision.Sep 22 2020, 10:40 PM

A lot of clang-format errors still exists. Looks okay otherwise.

This revision is now accepted and ready to land.Sep 22 2020, 10:40 PM

I will fix the Clang errors that won't cause incompatibilities and that aren't pointless. Thanks!