[prev in list] [next in list] [prev in thread] [next in thread]
List: cfe-commits
Subject: Re: [PATCH] Use ArrayRef in SemaInit.cpp.
From: Jordan Rose <jordan_rose () apple ! com>
Date: 2013-04-30 20:42:34
Message-ID: C919AF4C-6FB0-4D88-967C-3FCAF0EA4273 () apple ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
On Apr 30, 2013, at 12:45 , Sean Silva <silvas@purdue.edu> wrote:
>
> On Tue, Apr 30, 2013 at 1:55 PM, David Blaikie <dblaikie@gmail.com> wrote:
> * should we consider adding an ArrayRef implicit ctor from "None"?
> (see llvm/ADT/None.h and its use in llvm/ADT/Optional.h) Then you
> could replace all the "ArrayRef<T>()" calls with "None" (you might
> even want to do a sed replace on existing instances of this as a
> separate patch (a purely mechanical patch is easy to review/apply)).
>
> What's wrong with `ArrayRef<T>()`? It's explicit and clear. What is the benefit of \
> writing `None` instead? IMHO if I saw "None" that would just confuse me and send me \
> on a wild goose chase that would eventually terminate on finding the implicit \
> ArrayRef ctor and then saying to myself "why the heck didn't they just write \
> `ArrayRef<T>`"?
I like None. In argument lists with possibly empty arrays, specifying the empty array \
by type is weird and brittle. Normally, in an argument list you have values:
invalidate(ConstArgs, NonConstArgs);
and if one of those arrays is empty, you start mixing values and types when you're \
reading:
invalidate(ArrayRef<SVal>(), NonConstArgs);
Not to mention that if you decide to change the type of this thing, and it's threaded \
through multiple functions, you'd normally only have to change the function \
headers...but now you have to change some (but not all) of the call sites as well:
invalidate(ArrayRef<const MemRegion *>(), NonConstArgs);
even though that particular caller is still semantically doing the same thing:
invalidate(None, NonConstArgs);
It's really the same argument as "why not Optional<T>"? I personally find None much \
nicer than Optional<T>.
Jordan
[Attachment #5 (text/html)]
<html><head><meta http-equiv="Content-Type" content="text/html \
charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: \
space; -webkit-line-break: after-white-space; "><br><div><div>On Apr 30, 2013, at \
12:45 , Sean Silva <<a href="mailto:silvas@purdue.edu">silvas@purdue.edu</a>> \
wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div \
dir="ltr"><br><div class="gmail_extra"><div class="gmail_quote">On Tue, Apr 30, 2013 \
at 1:55 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" \
target="_blank">dblaikie@gmail.com</a>></span> wrote:<blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex">
* should we consider adding an ArrayRef implicit ctor from "None"?<br>
(see llvm/ADT/None.h and its use in llvm/ADT/Optional.h) Then you<br>
could replace all the "ArrayRef<T>()" calls with "None" (you might<br>
even want to do a sed replace on existing instances of this as a<br>
separate patch (a purely mechanical patch is easy to \
review/apply)).</blockquote><div><br></div><div style="">What's wrong with \
`ArrayRef<T>()`? It's explicit and clear. What is the benefit of writing `None` \
instead? IMHO if I saw "None" that would just confuse me and send me on a wild goose \
chase that would eventually terminate on finding the implicit ArrayRef ctor and then \
saying to myself "why the heck didn't they just write `ArrayRef<T>`"?</div> \
</div></div></div></blockquote><br></div><div>I like None. In argument lists with \
possibly empty arrays, specifying the empty array <i>by type</i> is weird and \
brittle. Normally, in an argument list you have \
values:</div><div><br></div><div>invalidate(ConstArgs, \
NonConstArgs);</div><div><br></div><div>and if one of those arrays is empty, you \
start mixing values and types when you're \
reading:</div><div><br></div><div>invalidate(ArrayRef<SVal>(), \
NonConstArgs);</div><div><br></div><div>Not to mention that if you decide to change \
the type of this thing, and it's threaded through multiple functions, you'd normally \
only have to change the function headers...but now you have to change some (but not \
all) of the call sites as well:</div><div><br></div><div>invalidate(ArrayRef<const \
MemRegion *>(), NonConstArgs);</div><div><br></div><div>even though that \
particular caller is still semantically doing the same \
thing:</div><div><br></div><div>invalidate(None, \
NonConstArgs);</div><div><br></div><div>It's really the same argument as "why not \
Optional<T>"? I personally find None much nicer than \
Optional<T>.</div><div><br></div><div>Jordan</div><br></body></html>
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic