[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

Vlad Serebrennikov via cfe-commits cfe-commits at lists.llvm.org
Mon Mar 11 09:04:54 PDT 2024


================
@@ -0,0 +1,67 @@
+//===----- SemaOpenACC.h - Semantic Analysis for OpenACC constructs -------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+/// \file
+/// This file declares semantic analysis for OpenACC constructs and
+/// clauses.
+///
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_SEMA_SEMAOPENACC_H
+#define LLVM_CLANG_SEMA_SEMAOPENACC_H
+
+#include "clang/AST/DeclGroup.h"
+#include "clang/Basic/OpenACCKinds.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Sema/Ownership.h"
+
+namespace clang {
+
+class Sema;
----------------
Endilll wrote:

> (Again, nitpicking because this is a precedent)

Your feedback is much appreciated! No worries about nitpicking, as we indeed establish a precedent here.

> In which case, I think we should #include "Sema.h" instead of a forward declaration. Forward declarations should be used in the cases where they matter, but otherwise humans waste effort trying to preserve this unneeded property.
>
> Also, #including "Sema.h" makes it clear that the reverse inclusion is not allowed for layering reasons, which is much more important. I can imagine these being introduced by someone wanting to write inline functions in Sema.h that cross component boundaries.

While it would be nice to have layering in Sema, and inline functions in `Sema` have already caused me some headaches, I don't think we can include `Sema.h` in `SemaOpenACC.h` (or vise-versa, for that matter), as it defeats one of the primary goals of this patch: making dependencies between parts of `Sema` visible. If we were to include `Sema.h` in `SemaOpenACC.h`, parts that depend on `SemaOpenACC` would implicitly have access to the entirety of `Sema`.

> Do we expect that clients realistically use SemaOpenACC but not Sema::OpenACC()? Do we want to encourage clients to accept/store SemaFoo& and SemaBar& instead of Sema&?
> 
> I think we should accept that at least for now, clients will and should still use Sema as the god-object, and that their benefit is not having to pay the compile time of #include "SemaUnrelated.h" or the human time of reading its contents.
> This is the pattern that client code is being migrated to in this patch, and it's much more amenable to mechanical migration.

I wonder if @erichkeane can give us his insight here, as he has more code dealing with OpenACC down the line.

https://github.com/llvm/llvm-project/pull/84184


More information about the cfe-commits mailing list