This is an archive of the discontinued LLVM Phabricator instance.

[Sema][ObjC] Add support for attribute "noescape"
ClosedPublic

Authored by ahatanak on Apr 18 2017, 7:14 PM.

Details

Summary

This patch adds support for attribute "noescape", which is used to tell the compiler that a block passed to a function will not be called after the function returns.

To ensure that the block does not escape, clang imposes the following restrictions on its usage:

  • Cannot be passed to a function expecting an escaping block
  • Cannot be returned from a function
  • Cannot be captured by a block
  • Cannot be assigned to a variable

There are other improvements we can make to Sema and CodeGen if the compiler can tell whether a block escapes or not, which should follow this patch.

rdar://problem/19886775

Diff Detail

Event Timeline

ahatanak created this revision.Apr 18 2017, 7:14 PM
aaron.ballman added inline comments.Apr 19 2017, 7:21 AM
include/clang/Basic/Attr.td
1399

I do not think GCC supports this attribute, so that should be using a GNU spelling rather than a GCC spelling.

1400

No need to specify the last two arguments, they should be handled by default already.

include/clang/Basic/AttrDocs.td
118

Please wrap to 80 columns.

"block parameter" is a bit strange -- I assumed that meant parameter to a block, not a function parameter of block type. May want to clarify.

Should also clarify "the block will not be invoked after the function returns." Does this code produce undefined behavior?

typedef void (^BlockTy)();
void nonescapingFunc(__attribute__((noescape)) BlockTy);
void escapingFunc(BlockTy);

void callerFunc(__attribute__((noescape)) BlockTy block) {
  nonescapingFunc(block); // OK
  escapingFunc(block);    // error: parameter is not annotated with noescape
}

void f(void) {
  BlockTy Blk = ^{};
  callerFunc(Blk);
  Blk();
}
include/clang/Basic/DiagnosticSemaKinds.td
3164

expecting an escaping argument

Also, someday we should make non-foo vs nonfoo a consistent decision in our diagnostics. We seem to use both forms.

3176

Should quote noescape.

lib/Sema/SemaChecking.cpp
2545 ↗(On Diff #95679)

Space before the asterisk.

2548 ↗(On Diff #95679)

Please don't use auto here.

2559 ↗(On Diff #95679)

Don't use auto here.

2562 ↗(On Diff #95679)

No need to use getName() here, you can just pass in the declaration directly. That means you should also remove the quotes from the diagnostic so it does not get double-quoted.

2567 ↗(On Diff #95679)

What about functions that have no prototype (they're not variadic, but they accept any number of arguments)? Should include a test for this.

lib/Sema/SemaDeclAttr.cpp
1521–1523

Is there ever a case where this if statement will early return? I think you can just use cast<ParmVarDecl>(D)->getType() below.

lib/Sema/SemaExpr.cpp
11374

Why dyn_cast_or_null? I didn't think IgnoreImpCasts() could return null.

11375

Don't use auto.

11378

Can drop the getName() call.

13897

Can drop the getName() call.

lib/Sema/SemaStmt.cpp
3203 ↗(On Diff #95679)

Don't use auto.

3205 ↗(On Diff #95679)

Can drop getName().

This doesn't forbid assigning them to block properties... is that intentional?

typedef void (^Block)(int);

@interface Foo
@property Block B;
@end

void foo(Foo *f, Block __attribute__((noescape)) b) {
  f.B = b;
}
include/clang/Basic/AttrDocs.td
123

Is it helpful to forbid taking the address of them, too?

TheRealAdamKemp added inline comments.
include/clang/Basic/AttrDocs.td
122

This may be too restrictive in some cases. Consider this:

typedef void (^BlockTy)();
void nonescapingFunc(__attribute__((noescape)) BlockTy);

void callerFunc(__attribute__((noescape)) BlockTy block) {
  nonescapingFunc(^{ block(); });
}

It sounds like the above code would give an error, but the capturing block is also nonescaping, which means it should be allowed. This is a useful pattern, and disallowing it would make these attributes very cumbersome.

The code could also be written like this:

typedef void (^BlockTy)();
void nonescapingFunc(__attribute__((noescape)) BlockTy);

void callerFunc(__attribute__((noescape)) BlockTy block) {
  BlockTy wrapBlock = ^{
    block();
  };
  nonescapingFunc(wrapBlock);
}

Again, I would expect that to not give an error or at least be able to decorate the declaration of wrapBlock to indicate that it is also nonescaping (can this attribute be applied to locals or only function arguments?).

rjmccall edited edge metadata.Apr 19 2017, 11:09 AM

The rule we actually want is that no reference to the block derived from the parameter value will survive after the function returns. You can include examples of things that would cause potential escapes, but trying to actually lock down the list seems ill-advised.

Are you sure you want to try to enforce this in the frontend? In Swift, we discovered that we needed some way of escaping the static checking in order to make this practical, because there are a lot of patterns that locally look like escapes but really aren't (e.g. passing the block to dispatch_async and then waiting for that to complete). Seems more like a static analysis than a frontend warning.

This doesn't forbid assigning them to block properties... is that intentional?

typedef void (^Block)(int);

@interface Foo
@property Block B;
@end

void foo(Foo *f, Block __attribute__((noescape)) b) {
  f.B = b;
}

No, it's not intentional. The code to check assignments of noescape blocks probably had to go to ActOnBinOp or BuildBinOp to forbid this too.

include/clang/Basic/AttrDocs.td
122

I didn't think forbidding capturing noescape blocks would be too restrictive in practice, but I see your point. If it's a common pattern, we probably shouldn't forbid that.

lib/Sema/SemaChecking.cpp
2567 ↗(On Diff #95679)

You are right, this assertion should check for functions without prototypes too.

The rule we actually want is that no reference to the block derived from the parameter value will survive after the function returns. You can include examples of things that would cause potential escapes, but trying to actually lock down the list seems ill-advised.

Are you sure you want to try to enforce this in the frontend? In Swift, we discovered that we needed some way of escaping the static checking in order to make this practical, because there are a lot of patterns that locally look like escapes but really aren't (e.g. passing the block to dispatch_async and then waiting for that to complete). Seems more like a static analysis than a frontend warning.

I'm actually not so sure we want to enforce this in the front-end. I was following what I thought Swift was doing, which is to reject code that can potentially cause noescape closures to escape. But that might be too restrictive and can reject legitimate code in some cases as Adam pointed out. I agree that static analysis would probably be better at detecting usages of noescape blocks that are likely to cause them to escape.

Do you think we should remove all the restrictions I listed and instead count on the users to ensure noescape blocks do not escape?

The rule we actually want is that no reference to the block derived from the parameter value will survive after the function returns. You can include examples of things that would cause potential escapes, but trying to actually lock down the list seems ill-advised.

Are you sure you want to try to enforce this in the frontend? In Swift, we discovered that we needed some way of escaping the static checking in order to make this practical, because there are a lot of patterns that locally look like escapes but really aren't (e.g. passing the block to dispatch_async and then waiting for that to complete). Seems more like a static analysis than a frontend warning.

I'm actually not so sure we want to enforce this in the front-end. I was following what I thought Swift was doing, which is to reject code that can potentially cause noescape closures to escape. But that might be too restrictive and can reject legitimate code in some cases as Adam pointed out. I agree that static analysis would probably be better at detecting usages of noescape blocks that are likely to cause them to escape.

Do you think we should remove all the restrictions I listed and instead count on the users to ensure noescape blocks do not escape?

Yes, I think so. You'll need to document that, of course.

John.

ahatanak updated this revision to Diff 96637.Apr 25 2017, 3:10 PM
ahatanak marked 4 inline comments as done.

Address review comments.

include/clang/Basic/AttrDocs.td
118

The code above doesn't produce undefined behavior as long as escapingFunc doesn't cause the block to escape. It is OK to declare a variable to hold the block and invoke it later after callerFunc returns. If a block literal is directly passed to callerFunc, the block will not be invoked after the function returns.

void f(void) {
  BlockTy Blk = ^{};
  callerFunc(Blk); // Blk can be invoked after callerFunc returns.
  Blk();
  callerFunc(^{}); // the block passed cannot be invoked after callerFunc returns.
}
aaron.ballman edited edge metadata.May 1 2017, 2:50 PM

The attribute is not used anywhere; the initial utility should be part of the patch introducing the attribute, unless that utility makes the patch very large.

lib/Sema/SemaDeclAttr.cpp
1523–1524

I don't think the type checking here is correct, at least according to the documentation. For instance, you don't appear to care whether the parameter is an array of block pointers vs an array of ints. Similar for the other composite types.

test/SemaObjCXX/noescape.mm
9

You should also have a test ensuring the attribute diagnoses when given an argument.

I agree with Aaron, it would be nice if this had some immediate effect. One obvious suggestion: take advantage of it in code generation. LLVM already has a parameter attribute called "nocapture" which conveys exactly the right semantics for noescape pointers and references. For block pointers, nocapture is a weaker statement than noescape, because noescape also restricts block copies, but it's still correct.

include/clang/Basic/AttrDocs.td
120

The implementation in Sema allows normal pointers and references in addition to block pointers. Assuming that's intended, it should be documented.

You should also document that *copies* of the block are also not allowed to escape. That's special behavior for block pointers.

144

Typo.

lib/Sema/SemaDeclAttr.cpp
1523–1524

A member pointer is not really like the others. Member pointers are always global constants, like function pointers; there's no meaningful scope for them to escape from.

Also, parameters never have array type. The type of a ParmVarDecl will be the decayed type, i.e. a pointer.

Also, you should check for an invalid decl and avoid checking the type, because that's likely a cascading failure. Test case: make a parameter with a bad type specifier.

ahatanak updated this revision to Diff 106735.Jul 14 2017, 5:08 PM
ahatanak marked 3 inline comments as done.

Address review comments.

  • Allow attaching "noescape" to pointers other than block pointers. Update the docs accordingly.
  • Attach attribute "nocapture" to parameters that are annotated with "noescape".
  • Call Sema::isValidPointerAttrType to determine whether "noescape" can be applied to a parameter. Also, use the existing warning "warn_attribute_pointers_only" rather than defining a new warning that will be used just for noescape.

I also thought about what else we can do in the front-end when a parameter has 'noescape". One idea is to do something similar to what r301667 did and omit emitting retains and releases of block captures when the block is passed to a function taking a noescape parameter. If that is viable, I can look into it after committing this patch.

Address review comments.

  • Allow attaching "noescape" to pointers other than block pointers. Update the docs accordingly.
  • Attach attribute "nocapture" to parameters that are annotated with "noescape".
  • Call Sema::isValidPointerAttrType to determine whether "noescape" can be applied to a parameter. Also, use the existing warning "warn_attribute_pointers_only" rather than defining a new warning that will be used just for noescape.

I also thought about what else we can do in the front-end when a parameter has 'noescape". One idea is to do something similar to what r301667 did and omit emitting retains and releases of block captures when the block is passed to a function taking a noescape parameter. If that is viable, I can look into it after committing this patch.

Hmm. Unfortunately, I'm not sure that's valid. The retains and releases of block captures don't protect against anything related to escaping the block; they protect against the original variables being modified during the lifetime of the block. It is true that a block literal passed to a noescape parameter has a shorter effective lifetime — we know for certain that it will be unused after the call, and ARC only promises that a value in an imprecise-lifetime strong variable like a block capture will be valid until the last load of that value from the variable. But that duration is still long enough for someone to modify the original variable in a way that is properly sequenced after the formation of the block.

Now, if you could prove that the variable was not modified for the duration of the call, that would be sufficient. And that would be easy to do in the common case by just proving that the address of the variable never escapes. Unfortunately, we don't have that information readily available because Sema doesn't collect it.

There are some other ways you could optimize blocks that are known not to escape, though. One big caveat is that you have to make sure the block behaves reasonably in response to being copied, because being noescape doesn't guarantee that the callee won't try to copy the block. However:

  • Copying a global block is always a no-op. If you gave the non-escaping stack block a global block isa, that would allow the blocks runtime to avoid doing extra work when a non-escaping block is spuriously copied, and it would allow the compiler to completely avoid emitting copy and destroy helpers for the block. Please clear this with Greg Parker first, though.
  • Even if that's not possible, you can still avoid emitting copy and destroy helpers in the common case where none of the captures are address-sensitive, i.e. __weak references or non-trivally-copyable C++ types, because the memcpy from the original block will be sufficient for correctness.
  • You can just capture a reference to an outer block instead of copying anything that it captures.
  • block variables which are only captured by non-escaping blocks are themselves known not to escape. (But remember that a non-escaping block can create an escaping block that captures the block variable!) Because block variable copies are only ever kicked off by block copy helpers, and you won't be generating those (or at least won't be asking them to copy your block variables for you), you also know that such variables can't be copied. That in turn means you can completely drop all the nonsense that only exists to supporting the lazy copying of block variables, like the block header and copy/destroy helpers and forwarding. Just generate the variable like a normal local variable and capture it by reference.

John.

include/clang/AST/Type.h
3157

Oh! I hadn't noticed that you were adding this to ExtParameterInfo. You're modifying the function type system; there's a lot of complexity to do that properly which you haven't done in this patch at all. That's especially true because, unlike all these other ExtParameterInfo cases, there's a natural subtyping rule for escaping parameters: a function type that takes a non-escaping parameter should be implicitly convertible to be a function type that takes an escaping parameter. You will also need to update a bunch of things, including the type printer, the mangler, the C++ function conversion rules, the C type compatibility rules, the redeclaration type matcher, and maybe even template argument deduction. It's a big thing to do.

You will want Sema tests in C, ObjC, C++, and ObjC++.

lib/CodeGen/CGCall.cpp
2100

You should make sure that building a CGFunctionInfo with an ObjCMethodDecl* collects this from the parameters. (And add tests!)

For blocks, at least, the ObjC method case is arguably more important than the C function case.

ahatanak marked an inline comment as done.Jul 24 2017, 4:25 PM

Address review comments.

  • Allow attaching "noescape" to pointers other than block pointers. Update the docs accordingly.
  • Attach attribute "nocapture" to parameters that are annotated with "noescape".
  • Call Sema::isValidPointerAttrType to determine whether "noescape" can be applied to a parameter. Also, use the existing warning "warn_attribute_pointers_only" rather than defining a new warning that will be used just for noescape.

I also thought about what else we can do in the front-end when a parameter has 'noescape". One idea is to do something similar to what r301667 did and omit emitting retains and releases of block captures when the block is passed to a function taking a noescape parameter. If that is viable, I can look into it after committing this patch.

Hmm. Unfortunately, I'm not sure that's valid. The retains and releases of block captures don't protect against anything related to escaping the block; they protect against the original variables being modified during the lifetime of the block. It is true that a block literal passed to a noescape parameter has a shorter effective lifetime — we know for certain that it will be unused after the call, and ARC only promises that a value in an imprecise-lifetime strong variable like a block capture will be valid until the last load of that value from the variable. But that duration is still long enough for someone to modify the original variable in a way that is properly sequenced after the formation of the block.

Now, if you could prove that the variable was not modified for the duration of the call, that would be sufficient. And that would be easy to do in the common case by just proving that the address of the variable never escapes. Unfortunately, we don't have that information readily available because Sema doesn't collect it.

OK, so I guess the optimization isn't valid in the following case, for example:

void foo3(id);

id __strong *gp;

void foo2(id a) {
  gp = &a;
  nonescapingFunc(^{ foo3(a); }); // this function can modify "a" before the block is executed.
}

There are some other ways you could optimize blocks that are known not to escape, though. One big caveat is that you have to make sure the block behaves reasonably in response to being copied, becase being noescape doesn't guarantee that the callee won't try to copy the block. However:

I didn't understand the following optimizations. I thought clang doesn't emit copy and destroy helper for global blocks with or without noescape?

  • Copying a global block is always a no-op. If you gave the non-escaping stack block a global block isa, that would allow the blocks runtime to avoid doing extra work when a non-escaping block is spuriously copied, and it would allow the compiler to completely avoid emitting copy and destroy helpers for the block. Please clear this with Greg Parker first, though.
  • Even if that's not possible, you can still avoid emitting copy and destroy helpers in the common case where none of the captures are address-sensitive, i.e. __weak references or non-trivally-copyable C++ types, because the memcpy from the original block will be sufficient for correctness.
  • You can just capture a reference to an outer block instead of copying anything that it captures.
  • block variables which are only captured by non-escaping blocks are themselves known not to escape. (But remember that a non-escaping block can create an escaping block that captures the block variable!) Because block variable copies are only ever kicked off by block copy helpers, and you won't be generating those (or at least won't be asking them to copy your block variables for you), you also know that such variables can't be copied. That in turn means you can completely drop all the nonsense that only exists to supporting the lazy copying of block variables, like the block header and copy/destroy helpers and forwarding. Just generate the variable like a normal local variable and capture it by reference.

John.

include/clang/AST/Type.h
3157

Rather than adding an enum to ExtParameterInfo and modifying the type system, I made changes in IRGen to find out whether a function parameter is annotated with noescape. I'm not sure when it is OK or not OK to change the type system when adding support for an attribute, but this seemed to be the right direction since most of the other attributes are handled this way.

lib/CodeGen/CGCall.cpp
2100

Should we issue a warning when a parameter of an ObjC method is annotated with noescape and the corresponding parameter of the overriding method in a derived class isn't? Also, should we warn about inconsistent C++ virtual functions too?

ahatanak updated this revision to Diff 107993.Jul 24 2017, 4:36 PM
ahatanak marked an inline comment as done.

Address review comments.

Hmm. Unfortunately, I'm not sure that's valid. The retains and releases of block captures don't protect against anything related to escaping the block; they protect against the original variables being modified during the lifetime of the block. It is true that a block literal passed to a noescape parameter has a shorter effective lifetime — we know for certain that it will be unused after the call, and ARC only promises that a value in an imprecise-lifetime strong variable like a block capture will be valid until the last load of that value from the variable. But that duration is still long enough for someone to modify the original variable in a way that is properly sequenced after the formation of the block.

Now, if you could prove that the variable was not modified for the duration of the call, that would be sufficient. And that would be easy to do in the common case by just proving that the address of the variable never escapes. Unfortunately, we don't have that information readily available because Sema doesn't collect it.

OK, so I guess the optimization isn't valid in the following case, for example:

void foo3(id);

id __strong *gp;

void foo2(id a) {
  gp = &a;
  nonescapingFunc(^{ foo3(a); }); // this function can modify "a" before the block is executed.
}

Right.

There are some other ways you could optimize blocks that are known not to escape, though. One big caveat is that you have to make sure the block behaves reasonably in response to being copied, becase being noescape doesn't guarantee that the callee won't try to copy the block. However:

I didn't understand the following optimizations. I thought clang doesn't emit copy and destroy helper for global blocks with or without noescape?

They're not really about global blocks, they're about whether we can emit a non-escaping block using the isa that we use for global blocks, which causes copies to be no-ops (because real global blocks do not have local captures).

John.

include/clang/AST/Type.h
3157

So, I do think it's acceptable to change the type system here; it's just that doing so definitely makes the change more complicated.

The guidance here is:

  • If it changes the basic rules for making a call, it really *must* be preserved as part of the function type, or else basic things like taking the address of the function will never work correctly. That's why everything currently in ExtParameterInfo is there — ns_consumed affects the balancing rules for how ARC makes calls, the Swift parameter-ABI treatments are part of the CC, etc.
  • Otherwise, it really comes down to how much we care about the feature working with indirection. I feel like blocks are probably the strongest motivator here because they're always invoked indirectly: if you want the feature to work on a block parameter, it really needs to be part of the function type, because we do not want to get into the business of finding non-canonical sources of information for function types, e.g. param decls from the TypeSourceInfo.

Overall, I would say that it tends to be the case that we eventually find attribute-based analyses limiting. If we start doing serious optimizations based on noescape, it is probably something that ought to go in the type system.

lib/CodeGen/CGCall.cpp
2100

Mmm. Probably yes in both cases, although we could alternatively just silently propagate the attribute InheritableParamAttr-style.

lib/CodeGen/CGClass.cpp
2086 ↗(On Diff #107993)

Typo.

ahatanak updated this revision to Diff 109814.Aug 4 2017, 1:41 PM

Include noescape attribute information in the function prototype and modify the AST printer and name mangling accordingly.

Also, make changes to allow implicit conversion from a function that takes noescape parameters to a function that doesn't. For example:

void (*fnptr0)(int *) = &noescapefunc; valid conversion.
void (*fnptr1)(attribute((noescape)) int *) = &escapefunc;
invalid conversion.

lib/CodeGen/CGCall.cpp
2100

In the new patch, I chose to print diagnostics when the overriding method is missing noescape on its parameters lest users unintentionally let pointers escape.

rjmccall added inline comments.Aug 5 2017, 5:26 PM
include/clang/Basic/AttrDocs.td
138
Additionally, when the parameter is a `block pointer
<https://clang.llvm.org/docs/BlockLanguageSpec.html>`, the same restriction applies
to copies of the block.  For example:
include/clang/Sema/Sema.h
1480

I assume this only returns noescape from parameters that are not noescape in ToType.

lib/AST/ItaniumMangle.cpp
2700

This is not the right place to do this:

  • It needs to be mangled in function types, not just on function declarations. It looks like the code here supporting pass_object_size just gets this wrong.
  • We don't want to mangle this in top-level declarations because doing so will make it more directly ABI-breaking to retroactively add this attribute to a declaration.

Instead, you should handle it in mangleExtParameterInfo. Because these qualifiers are mangled in reverse alphabetical order, and "noescape" precedes "ns_consumed", it should be the last thing tested for in that function.

lib/Sema/SemaDeclCXX.cpp
14024

I think we generally encourage the uses of braces when the nested statement is this complex, even if it is technically a single statement.

lib/Sema/SemaDeclObjC.cpp
186

I know this is existing code, but you should probably at least warn about ns_consumed mismatches here in non-ARC.

lib/Sema/SemaExpr.cpp
7180

So this function removes noescape attributes from FromType to match ToType, but only if it's a legal conversion? Maybe it would be more reasonable to report an inability to convert in a more direct way, like by returning a null type.

Also, I think you're much more likely to avoid implicitly stripping sugar if you take both of these (or at least FromType?) as a QualType, and also return a QualType. Or you could pass the original QualType down separately and just return that in the cases where you don't need to change anything.

7251

I think the right place to do this is probably mergeFunctionTypes.

lib/Sema/SemaTemplate.cpp
5477

I think this is not the right place to be doing this; instead, we should be applying a conversion to the argument expression in CheckTemplateArgument. It looks like we already do that in C++17 mode, because C++17 broadened the rules here to allow an arbitrary function conversion (among the other things allowed by the "converted constant expression" rules, [temp.arg.nontype]p2). We probably could just leave it at that, since earlier standards are (in their infinite wisdom) stricter about type-matching here. If that poses a serious compatibility problem, the right fix is to modify the clause in CheckTemplateArgument for non-type template parameters where it deals with pointers, references, and member pointers to functions to specifically allow this conversion. But I'll note that we don't seem to be similarly generous about noreturn.

ahatanak updated this revision to Diff 110612.Aug 10 2017, 11:47 AM
ahatanak marked 6 inline comments as done.

Address review comments.

lib/AST/ItaniumMangle.cpp
2700

Oh that's right. We should avoid breaking ABI when retroactively adding noescape.

lib/Sema/SemaDeclCXX.cpp
14024

I assume you are suggesting using braces for the if statement, not the for loop?

lib/Sema/SemaDeclObjC.cpp
186

I made warn_nsconsumed_attribute_mismatch and warn_nsreturns_retained_attribute_mismatch warnings that get promoted to errors when ARC is enabled.

lib/Sema/SemaExpr.cpp
7180

Yes. that's correct. The original implementation of the function removed noescape attributes from FromType's parameters to match ToType, but only if doing so made it a legal conversion.

I made changes to have this function take and return QualType and return QualType() when noescape on parameters makes the conversion invalid.

7251

When we have the following conditional operator expression, what is the type of the expression?

void func0(__attribute__((noescape)) int *a, int *b);
void func1(int *a, __attribute__((noescape)) int *b);

c ? func0 : func1;

The standard says the type of each parameter in the composite parameter type list is the composite type of the corresponding parameters of both functions, so I guess mergeFunctionType should drop noescape from both parameters?

lib/Sema/SemaTemplate.cpp
5477

I removed this code so that noescape is handled the same way noreturn is handled, which causes clang to complain about "S4<&noescapeFunc2> e1;" in test/SemaObjCXX/noescape.mm when it is not compiling for c++17 or later.

Is that what you meant?

rjmccall added inline comments.Aug 10 2017, 12:05 PM
lib/Sema/SemaDeclCXX.cpp
14024

Yes, exactly. This looks better, thank you.

lib/Sema/SemaExpr.cpp
7174

getAs<>, please.

7185

There's a potential for an illegal conversion here if ToProto->hasExtParameterInfos().

7251

Yes, that sounds right.

ahatanak updated this revision to Diff 115432.Sep 15 2017, 11:24 AM

Address review comments.

The new patch defines a new function "mergeExtParameterInfo" that merges two functions' ExtParameterInfo lists and uses it where previously doFunctionTypesMatchOnExtParameterInfos was used.

lib/Sema/SemaExpr.cpp
7251

I defined a new function (mergeExtParameterInfo) that checks the compatibility of two functions and creates the composite type and used it for C's compatibility checking and C++'s conversion rules. Also, I added new test cases in test/Sema/noescape.c.

Thanks, that looks good.

This revision was automatically updated to reflect the committed changes.
ahatanak reopened this revision.Sep 19 2017, 11:37 PM

Reopening this review. I accidentally closed this review when I committed r313717 (the patch that adds support for 'noescape').

ahatanak accepted this revision.Sep 19 2017, 11:38 PM

This one can be closed.

This revision is now accepted and ready to land.Sep 19 2017, 11:38 PM
ahatanak closed this revision.Sep 19 2017, 11:39 PM