<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:x="urn:schemas-microsoft-com:office:excel" xmlns:p="urn:schemas-microsoft-com:office:powerpoint" xmlns:a="urn:schemas-microsoft-com:office:access" xmlns:dt="uuid:C2F41010-65B3-11d1-A29F-00AA00C14882" xmlns:s="uuid:BDC6E3F0-6DA3-11d1-A2A3-00AA00C14882" xmlns:rs="urn:schemas-microsoft-com:rowset" xmlns:z="#RowsetSchema" xmlns:b="urn:schemas-microsoft-com:office:publisher" xmlns:ss="urn:schemas-microsoft-com:office:spreadsheet" xmlns:c="urn:schemas-microsoft-com:office:component:spreadsheet" xmlns:odc="urn:schemas-microsoft-com:office:odc" xmlns:oa="urn:schemas-microsoft-com:office:activation" xmlns:html="http://www.w3.org/TR/REC-html40" xmlns:q="http://schemas.xmlsoap.org/soap/envelope/" xmlns:rtc="http://microsoft.com/officenet/conferencing" xmlns:D="DAV:" xmlns:Repl="http://schemas.microsoft.com/repl/" xmlns:mt="http://schemas.microsoft.com/sharepoint/soap/meetings/" xmlns:x2="http://schemas.microsoft.com/office/excel/2003/xml" xmlns:ppda="http://www.passport.com/NameSpace.xsd" xmlns:ois="http://schemas.microsoft.com/sharepoint/soap/ois/" xmlns:dir="http://schemas.microsoft.com/sharepoint/soap/directory/" xmlns:ds="http://www.w3.org/2000/09/xmldsig#" xmlns:dsp="http://schemas.microsoft.com/sharepoint/dsp" xmlns:udc="http://schemas.microsoft.com/data/udc" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:sub="http://schemas.microsoft.com/sharepoint/soap/2002/1/alerts/" xmlns:ec="http://www.w3.org/2001/04/xmlenc#" xmlns:sp="http://schemas.microsoft.com/sharepoint/" xmlns:sps="http://schemas.microsoft.com/sharepoint/soap/" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:udcs="http://schemas.microsoft.com/data/udc/soap" xmlns:udcxf="http://schemas.microsoft.com/data/udc/xmlfile" xmlns:udcp2p="http://schemas.microsoft.com/data/udc/parttopart" xmlns:wf="http://schemas.microsoft.com/sharepoint/soap/workflow/" xmlns:dsss="http://schemas.microsoft.com/office/2006/digsig-setup" xmlns:dssi="http://schemas.microsoft.com/office/2006/digsig" xmlns:mdssi="http://schemas.openxmlformats.org/package/2006/digital-signature" xmlns:mver="http://schemas.openxmlformats.org/markup-compatibility/2006" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns:mrels="http://schemas.openxmlformats.org/package/2006/relationships" xmlns:spwp="http://microsoft.com/sharepoint/webpartpages" xmlns:ex12t="http://schemas.microsoft.com/exchange/services/2006/types" xmlns:ex12m="http://schemas.microsoft.com/exchange/services/2006/messages" xmlns:pptsl="http://schemas.microsoft.com/sharepoint/soap/SlideLibrary/" xmlns:spsl="http://microsoft.com/webservices/SharePointPortalServer/PublishedLinksService" xmlns:Z="urn:schemas-microsoft-com:" xmlns:st="" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv=Content-Type content="text/html; charset=utf-8">
<meta name=Generator content="Microsoft Word 12 (filtered medium)">
<style>
<!--
/* Font Definitions */
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
span.EmailStyle17
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;}
@page Section1
{size:612.0pt 792.0pt;
margin:72.0pt 72.0pt 72.0pt 72.0pt;}
div.Section1
{page:Section1;}
-->
</style>
<!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang=EN-GB link=blue vlink=purple>
<div class=Section1>
<p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'>Hi Chandler,<o:p></o:p></span></p>
<p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'><o:p> </o:p></span></p>
<p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'>All points noted, new patch incoming by the end of the day.<o:p></o:p></span></p>
<p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'><o:p> </o:p></span></p>
<p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'>Cheers,<o:p></o:p></span></p>
<p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'><o:p> </o:p></span></p>
<p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'>James<o:p></o:p></span></p>
<p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'><o:p> </o:p></span></p>
<div style='border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0cm 0cm 0cm'>
<p class=MsoNormal><b><span lang=EN-US style='font-size:10.0pt;font-family:
"Tahoma","sans-serif"'>From:</span></b><span lang=EN-US style='font-size:10.0pt;
font-family:"Tahoma","sans-serif"'> Chandler Carruth
[mailto:chandlerc@google.com] <br>
<b>Sent:</b> 26 January 2012 11:17<br>
<b>To:</b> James Molloy<br>
<b>Cc:</b> cfe-commits@cs.uiuc.edu; juli@clockworksquid.com;
ken.dyck@onsemi.com<br>
<b>Subject:</b> Re: [cfe-commits] [PATCH] Fix tag decls/enum constants in
function prototypes<o:p></o:p></span></p>
</div>
<p class=MsoNormal><o:p> </o:p></p>
<div>
<p class=MsoNormal>On Thu, Jan 26, 2012 at 3:01 AM, James Molloy <<a
href="mailto:james.molloy@arm.com">james.molloy@arm.com</a>> wrote:<o:p></o:p></p>
<div>
<div>
<p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'><span
style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>No
problem. This is a convention in LLVM’s test dirs</span><o:p></o:p></p>
</div>
</div>
<div>
<p class=MsoNormal><o:p> </o:p></p>
</div>
<div>
<p class=MsoNormal>Just as an FYI, I think the convention is going away even in
LLVM. Many new tests getting added don't have a date, and I'm hoping to see a
continuing trend away from them.<o:p></o:p></p>
</div>
<div>
<p class=MsoNormal><o:p> </o:p></p>
</div>
<blockquote style='border:none;border-left:solid #CCCCCC 1.0pt;padding:0cm 0cm 0cm 6.0pt;
margin-left:4.8pt;margin-right:0cm'>
<div>
<div>
<div>
<p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'><span
style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Separation
of concerns is good. Yes, I’m adding a new warning that has no flag, but it’s
essentially a clone of another warning with one verb different that currently
has no flag. Do you suggest I add the flag to cover both the old and new
warnings in the same patch? I can do it as a followup and I’d prefer that if
possible.</span><o:p></o:p></p>
</div>
</div>
</div>
</blockquote>
<div>
<p class=MsoNormal><o:p> </o:p></p>
</div>
<div>
<p class=MsoNormal>This is a tricky point. The thing is that this file is
specifically designed to prevent the slow creep for the reasons you describe.
The logic you use got us into a bad state where most warnings don't have flags.<o:p></o:p></p>
</div>
<div>
<p class=MsoNormal><o:p> </o:p></p>
</div>
<div>
<p class=MsoNormal>That said, I support separate commits, I would just invert
the order. Add the flag to the existing warning, and remove it from the file,
then proceed with this patch, using the newly available flag. That way the test
policy is maintained and it never shrinks. It's a bit of a refactoring tax, but
probably a healthy one considering how bad this got.<o:p></o:p></p>
</div>
<div>
<p class=MsoNormal> <o:p></o:p></p>
</div>
<blockquote style='border:none;border-left:solid #CCCCCC 1.0pt;padding:0cm 0cm 0cm 6.0pt;
margin-left:4.8pt;margin-right:0cm'>
<div>
<div>
<div>
<p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'><span
style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> </span>-
You need to add visiting logic for all of these decls to the various visitors I
suspect. We have several, I'd have to go looking to enumerate all of them. Off
the top of my head I would check:<o:p></o:p></p>
</div>
<div>
<div>
<p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'>
- RecursiveASTVisitor descends into these decls.<o:p></o:p></p>
</div>
<div>
<p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'>
- The various printing and dumping visitors catch them<o:p></o:p></p>
</div>
<div>
<p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'>
- The CFG builder handles them (not sure how much it really has to care)<o:p></o:p></p>
</div>
<div>
<p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'>
- Maybe update libclang? Not likely important for the first pass.<o:p></o:p></p>
</div>
</div>
<div>
<p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'><span
style='color:#1F497D'> </span><o:p></o:p></p>
<p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'><span
style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Thanks
for catching this – I didn’t think it would be required. I’ll ensure everything
acts as it should, although I’m not sure how much effect this will have. The
decls already existed previously, just were in the wrong scope.</span><o:p></o:p></p>
</div>
</div>
</div>
</blockquote>
<div>
<p class=MsoNormal><o:p> </o:p></p>
</div>
<div>
<p class=MsoNormal>Interesting... Maybe we're already visiting them somewhere
else, perhaps in the outer decl context? It'd be good to verify what's going on
here. It's unclear to me whether this requires no change, strictly an additive
change (visit these things), or moving the visit that exists to occur at the
right point in the visitation sequence....<o:p></o:p></p>
</div>
<div>
<p class=MsoNormal><o:p> </o:p></p>
</div>
<div>
<p class=MsoNormal>Looking at the output of AST dump with some obvious test
cases will likely make it obvious what the right thing is to do here.<o:p></o:p></p>
</div>
<div>
<p class=MsoNormal> <o:p></o:p></p>
</div>
<blockquote style='border:none;border-left:solid #CCCCCC 1.0pt;padding:0cm 0cm 0cm 6.0pt;
margin-left:4.8pt;margin-right:0cm'>
<div>
<div>
<div>
<div>
<p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'><span
style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>It’s
raw new’d for the reason you state. I didn’t use arrayref because the existing
code for ParmVarDecl (see diff context) also doesn’t use ArrayRef. I
copied its interface completely, assuming (for better or worse) that it was
designed that way for a reason.</span><o:p></o:p></p>
</div>
</div>
</div>
</div>
</blockquote>
<div>
<p class=MsoNormal><o:p> </o:p></p>
</div>
<div>
<p class=MsoNormal>Hmm, I fear this may not be a good assumption. ;]
Technically, ArrayRef isn't as good of a fit for the Parm ones because we have
external knowledge about how many of those there are. Using an arrayref would
waste at least 4 bytes of storage. Since you need both the pointer and the size,
might as well use ArrayRef.<o:p></o:p></p>
</div>
<div>
<p class=MsoNormal><o:p> </o:p></p>
</div>
<div>
<p class=MsoNormal>Here and below where the same issue arose, I would encourage
you to use the simplest and most minimal interface that works for your use
case. If the surrounding code has a more complex pattern that isn't actually
needed, let's simplify that as well to get consistency. (Naturally, I would do
that in a separate patch, it can even be another 'pre factoring' patch if you
will). I don't want us to become complacent in the pursuit of consistency and
miss opportunities to use better and/or simpler interfaces.<o:p></o:p></p>
</div>
</div>
</div>
</body>
</html>