[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D133586#3831624 <https://reviews.llvm.org/D133586#3831624>, @rmaz wrote:

> In D133586#3831618 <https://reviews.llvm.org/D133586#3831618>, @vsapsai wrote:
>
>> How correct is it to access `isConst`, `isVolatile`, `isRestrict` for `FunctionNoProtoType`? Yes, we can provide some default value but I'm curious if accessing that default value is correct.
>>
>> For the record, I've tried to fix the same problem in https://reviews.llvm.org/D104963 in a different way.
>
> That was my initial solution as well, but it seemed safer to ensure these methods always returned a consistent value without auditing all the possible call sites. I agree that it doesn't seem right that these methods are on the base class at all if they are only valid in one of the subclasses.

The trouble is -- `FunctionNoProtoType` is pretty rare to encounter and is getting more rare by the year now that C has removed the feature entirely, but you can't build a prototyped function from an unprototyped one or vice versa (their only relationship is that they're both function types with a known return type). So if we pushed the methods from `FunctionType` down into `FunctionProtoType`, we'd have to run through the casting machinery a lot more than we currently do which could potentially regress compile times for very little benefit.

Personally, I think the next step is to add a local `assert()` to this function to try to find out why we're calling this on functions without a prototype and fix up the call sites. I think the intent is that you should not be calling this function on an unprototyped function and it'd be good for debug builds to yell if we're doing that, but returning a default constructed object is a safe recovery for release builds.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133586/new/

https://reviews.llvm.org/D133586