The rest of the patch looks good by me. You might want to check with John if he has any comments. - Gao
________________________________________ From: Reid Kleckner [[email protected]] Sent: Wednesday, February 19, 2014 5:43 PM To: [email protected] Cc: John McCall; David Majnemer; Warren Hunt; [email protected] cfe; Gao, Yunzhong Subject: Re: [PATCH] Complete Rewrite of CGRecordLayoutBuilder On Wed, Feb 19, 2014 at 5:41 PM, Yunzhong Gao <[email protected]<mailto:[email protected]>> wrote: A coding style question: It seems that CGRecordLowering is exposing a lot of its member fields and functions. I wonder whether it makes sense to make some of these member fields private. For example, it seems std::vector<MemberInfo> Members does not need to be referenced by outside classes, and could be made private (and hence the definition of MemberInfo). I did not check all the "Input Memoization fields", but I suspect some of them do not need to be exposed either. And some of the member functions as well, such as, void lowerUnion(); and some other functions used by lower(), probably can be hidden from outside classes as well. I think this is minor point since CGRecordLowering is only defined and used in this file, and I am not entirely sure about LLVM's coding style on access control. What do the other reviewers think? Adding private: around everything except the entry points is probably a good idea, but I don't think it matters too much. All that stuff is TU local anyway. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
