danielmarjamaki added inline comments.
================ Comment at: include/clang/AST/ASTContext.h:42 #include "clang/Basic/Specifiers.h" +#include "clang/Basic/VersionTuple.h" #include "llvm/ADT/APSInt.h" ---------------- I don't see why this is included here. ================ Comment at: include/clang/AST/Mangle.h:55 const ManglerKind Kind; + // Used for cross tranlsation unit analysis. + // To reduce the risk of function name collision in C projects, we force ---------------- tranlsation => translation ================ Comment at: lib/AST/ASTContext.cpp:1457 + return nullptr; + for (Decl *D : DC->decls()) { + const auto *SubDC = dyn_cast<DeclContext>(D); ---------------- could add a "const" perhaps ================ Comment at: lib/AST/ASTContext.cpp:1491 + std::string MangledFnName = getMangledName(FD, MangleCtx.get()); + std::string ExternalFunctionMap = (XTUDir + "/externalFnMap.txt").str(); + ASTUnit *Unit = nullptr; ---------------- as far as I see ExternalFunctionMap can be moved into the subscope. ================ Comment at: lib/AST/ASTContext.cpp:1500 + std::string FunctionName, FileName; + while (ExternalFnMapFile >> FunctionName >> FileName) + FunctionFileMap[FunctionName] = (XTUDir + "/" + FileName).str(); ---------------- I would recommend that a parser is added that make sure the file data is correct/sane. If there is some load/save mismatch now or in the future or if the user choose to tweak the file for some reason, the behaviour could be wrong. ================ Comment at: lib/AST/ASTContext.cpp:1502 + FunctionFileMap[FunctionName] = (XTUDir + "/" + FileName).str(); + ExternalFnMapFile.close(); + } ---------------- well.. I believe it's redundant to close this explicitly since the std::ifstream destructor should do that. But it doesn't hurt doing it. ================ Comment at: lib/Basic/SourceManager.cpp:2028 /// \brief Determines the order of 2 source locations in the translation unit. +/// FIXME: It also works when two locations are from different translation unit. +/// In that case it will return *some* order. ---------------- I am not good at english. But I believe unit=>units. ================ Comment at: lib/Basic/SourceManager.cpp:2126 } - llvm_unreachable("Unsortable locations found"); + // FIXME: Source locations from different translation unit. + return LOffs.first < ROffs.first; ---------------- I am not good at english. But I believe unit=>units. ================ Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:48 +#include <sys/file.h> +#include <unistd.h> +#include <fstream> ---------------- I believe sys/file.h and unistd.h are posix includes. ================ Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:433 + return; + int fd = open(fileName.c_str(), O_CREAT|O_WRONLY|O_APPEND, 0666); + flock(fd, LOCK_EX); ---------------- this is posix file-I/O ================ Comment at: tools/clang-cmdline-arch-extractor/ClangCmdlineArchExtractor.cpp:56 + if (Sources.empty()) + return 1; + ---------------- Maybe use the EXIT_FAILURE instead ================ Comment at: tools/clang-cmdline-arch-extractor/ClangCmdlineArchExtractor.cpp:66 + for (StringRef SourceFile : Sources) { + char *Path = realpath(SourceFile.data(), nullptr); + if (Path) ---------------- This is posix function as far as I know. ================ Comment at: tools/clang-cmdline-arch-extractor/ClangCmdlineArchExtractor.cpp:72 + + return 0; +} ---------------- EXIT_SUCCESS is also possible however I guess that is 0 on all implementations. ================ Comment at: tools/clang-func-mapping/ClangFnMapGen.cpp:32 +#include <string> +#include <sys/file.h> +#include <unistd.h> ---------------- posix includes ================ Comment at: tools/clang-func-mapping/ClangFnMapGen.cpp:153 + + if (!FileName.empty()) + switch (FD->getLinkageInternal()) { ---------------- I do not see how FileName could be empty. It always starts with "/ast/" right? ================ Comment at: tools/clang-func-mapping/ClangFnMapGen.cpp:193 + lockedWrite(BuildDir + "/definedFns.txt", DefinedFuncsStr.str()); + std::stringstream CFGStr; + for (auto &Entry : CG) { ---------------- if a STL stream will be used I recommend the std::ostringstream instead ================ Comment at: tools/clang-func-mapping/ClangFnMapGen.cpp:244 + errs() << "Exactly one XTU dir should be provided\n"; + return 1; + } ---------------- maybe use EXIT_FAILURE ================ Comment at: tools/clang-func-mapping/ClangFnMapGen.cpp:257 + Tool.run(newFrontendActionFactory<MapFunctionNamesAction>().get()); +} ---------------- no return. ================ Comment at: tools/xtu-analysis/xtu-analyze.py:29 + +threading_factor = int(multiprocessing.cpu_count() * 1.5) +analyser_output_formats = ['plist-multi-file', 'plist', 'plist-html', ---------------- does this mean that if there are 4 cores this script will by default use 6 threads? isn't that too aggressive? Repository: rL LLVM https://reviews.llvm.org/D30691 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits