[PATCH] D77701: [Sema] refactor static functions into private methods NFC

Wed Apr 8 09:45:11 PDT 2020

aaron.ballman added a comment.

In D77701#1969687 <https://reviews.llvm.org/D77701#1969687>, @nickdesaulniers wrote:

> In D77701#1969132 <https://reviews.llvm.org/D77701#1969132>, @aaron.ballman wrote:
>
> > OTOH, this is reasonable, NFC, and I tend to agree about it being a code smell.
> >  OTOH, this makes parsing Sema.h that much slower and adds even more text for us to wade through in that header file.
> >
> > Given that the approaches are equivalent except for smell and that Sema.h is already 12kLoC long, I would rather leave this as-is or possibly even see us go in the opposite direction and use static functions whenever there's a private function only used in a single .cpp file and making it static doesn't make it overly awkward for some reason.
>
>
> Totally, this was a yak shave no one asked for.  I was looking at `DiagnoseNoDiscard` in D77611 <https://reviews.llvm.org/D77611>.  Fixing that case there then checking for other instances in the TU turned up more.  I pulled a thread that unwound the sweater.

Heh, yeah, I've been there before.

> I don't have any attachment to this patch, so I'm ok with it being rejected.  If there's concerns with Sema's compile time, please do consider splitting it up further though.

I am curious to hear what other reviewers think, so I wouldn't reject based on my observations alone. I think splitting sema.h would be an interesting prospect, but the bulk of the file are declarations within the `Sema` class, so splitting may be nontrivial.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77701