dcoughlin added a comment.

Except for some drive-by nits, this is a high-level review.

I have some high-level questions about the design of the library:

1. **How do you intend to handle the case when there are multiple function 
definitions with the same USR?** Whose responsibility it is to deal with this: 
the client (for example, the static analyzer) or the index/database 
(libCrossTU)? This seems to me to be a fundamental part of the design for this 
feature that is missing. If you expect the client to handle this scenario (for 
example, to pick a definition) I would expect the library API to return more 
than a single potential result for a query. If you expect the the library to 
pick a definition then at a minimum you should document how this will be done. 
If the library will treat this as an error then you should communicate this to 
the client so it can attempt to recover from it.

2. **Whose responsibility is it to handle errors that the library runs into?** 
Right now several errors appear to reported to the end user as diagnostics. It 
seems to me that the decision of how (or whether) to report these errors to the 
end user should be up to a particular client. Are these errors even actionable 
on the part of end users? My sense it that with the envisioned workflow users 
would not know/care about the format of the database file.

3. **Where should the code that understands the database file format live?** 
Right now the logic for the parsing of the index format is in libCrossTU and 
the emission of the index is in the command-line tool. I think it would be 
easier to maintain if there were a single place that understand the file 
format. This way consumers and emitters of the index could be easily updated 
when the format/representation changes. Additionally, I think it is important 
to document this format.



================
Comment at: include/clang/Basic/DiagnosticCrossTUKinds.td:13
+def err_fnmap_parsing : Error<
+  "error parsing CrossTU index file: '%0' line: %1 'USR filename' format "
+  "expected">;
----------------
Is this a user-facing diagnostic? Do we expect users to know what 'CrossTU' 
means? Or what 'USR' means?


================
Comment at: include/clang/CrossTU/CrossTranslationUnit.h:60
+  ///
+  /// Note that the AST files should also be in the \p CrossTUDir.
+  const FunctionDecl *getCrossTUDefinition(const FunctionDecl *FD,
----------------
Can this document what happens when the function declaration cannot be found? 
And what happens when multiple function declarations are found?


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:48
+                                                       StringRef LookupFnName) 
{
+  if (!DC)
+    return nullptr;
----------------
Should this assert that DC is not null? What is the reason to accept null here?


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:81
+      if (!ExternalFnMapFile) {
+        Context.getDiagnostics().Report(diag::err_fe_error_opening)
+            << ExternalFunctionMap << "required by the CrossTU functionality";
----------------
What is the rationale behind emitting this as a diagnostic? In general for a 
library I would expect that errors would be communicated to the client, which 
then would have responsibility for handling them, reporting them to the user, 
or aborting.


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:92
+        StringRef LineRef{Line};
+        if (Pos > 0 && Pos != std::string::npos) {
+          FunctionName = LineRef.substr(0, Pos);
----------------
It looks like this is parsing. Is the file format documented anywhere? Also, it 
would be good to keep the parsing and generation code  in the same place so 
that it is easy to figure out what to update when the file format changes.


================
Comment at: tools/clang-func-mapping/ClangFnMapGen.cpp:86
+          if (SM.isInMainFile(Body->getLocStart()))
+            DefinedFuncsStr << LookupName.str().str() << " " << CurrentFileName
+                            << "\n";
----------------
It seems like the functionality that writes an entry and the functionality that 
reads an entry should ideally be in the same place. This way when the format is 
updated it is obvious what other places need to be updated. Can the code that 
writes a mapping be moved to libCrossTU and can we have this tool link against 
it?


https://reviews.llvm.org/D34512



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to