<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Apr 5, 2017, at 05:44, Alex L <<a href="mailto:arphaman@gmail.com" class="">arphaman@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><br class="Apple-interchange-newline"><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="gmail_quote" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">On 5 April 2017 at 13:38, Duncan Exon Smith<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:dexonsmith@apple.com" target="_blank" class="">dexonsmith@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><span class=""><br class=""><br class="">> On Apr 5, 2017, at 05:13, Aaron Ballman via Phabricator <<a href="mailto:reviews@reviews.llvm.org" class="">reviews@reviews.llvm.org</a>> wrote:<br class="">><br class="">> aaron.ballman added inline comments.<br class="">><br class="">><br class="">> ================<br class="">> Comment at: lib/Sema/SemaAttr.cpp:578<br class="">> + return;<br class="">> + Diag(PragmaAttributeStack.<wbr class="">back().Loc, diag::warn_pragm_attribute_no_<wbr class="">pop_eof);<br class="">> +}<br class="">> ----------------<br class="">> arphaman wrote:<br class="">>> aaron.ballman wrote:<br class="">>>> Perhaps adding a FixIt here would be a kindness?<br class="">>> Where would the fix-it point to? I think only the user will know the location at which they meant to insert `#pragma clang attribute pop`.<br class="">> Given that it's a warning rather than an error, and our recovery mechanism is to effectively pop the pragma at the end of the TU, I was thinking it could be added at the end of the TU. However, you are correct that it's probably not where the programmer needs it,<br class=""><br class=""></span>What about at the end of the file the push is in? This is likely to be used in header files, and it's probably unintentional if it extends past the end of the file of the push.<br class=""></blockquote><div class=""><br class=""></div><div class="">Then we'd have to take into account preprocessor directives, as we'd obviously want to insert it before the '#endif'. Or if the user used `#pragma once` then the end of the file could probably work.</div><div class="">IMHO the improvement can be done as a follow-up that improves this and other pragmas like #pragma pack.</div><div class=""><br class=""></div></div></div></blockquote><div><br class=""></div><div>SGTM.</div><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><br class="">I think we should consider the same thing for #pragma pack (although that's a little off-topic).<br class=""><span class=""><br class="">> so a FixIt likely isn't appropriate. Does that suggest the warning should be an error instead?<br class=""><br class=""></span>Maybe it should be an error, but I still think a fixit would be nice if we can find a spot.<br class=""><div class="HOEnZb"><div class="h5"><br class="">><br class="">><br class="">> Repository:<br class="">> rL LLVM<br class="">><br class="">><span class="Apple-converted-space"> </span><a href="https://reviews.llvm.org/D30009" rel="noreferrer" target="_blank" class="">https://reviews.llvm.org/<wbr class="">D30009</a><br class="">><br class="">><br class="">></div></div></blockquote></div></div></blockquote></div><br class=""></body></html>