[PATCH] D14889: sancov -not-covered-functions.

[Mike Aizatsky via llvm-commits]

aizatsky added a comment.

ptal


================
Comment at: lib/Object/ObjectFile.cpp:118
@@ +117,3 @@
+
+Triple ObjectFile::getTriple() const {
+  llvm::Triple TheTriple("unknown-unknown-unknown");
----------------
pcc wrote:
> samsonov wrote:
> > pcc wrote:
> > > I'd probably prefer if this function returned a `Target *` to reflect the fact that it isn't really supposed to (nor can it) return an accurate triple but rather something that can be used to summon appropriate bits of MC etc.
> > Agree: you can't rely deduce triple from object file. At the very least, you should reflect that you're guessing the triple in method name.
> Actually it seems that since the MC constructors use the target triple string we should return a triple string here at least for now.
Sorry, can't do. Triple name is used to obtain various pieces of MC.
Plus target lookup changes Triple object (!). That's why I can't simply return std::string instead of Triple here. I've added the clarifying comment to the .h files about the function.

================
Comment at: tools/sancov/sancov.cc:157
@@ +156,3 @@
+    auto InliningInfo = Symbolizer.symbolizeInlinedCode(ClBinaryName, Addr);
+    FailIfError(InliningInfo);
+    for (uint32_t i = 0; i < InliningInfo->getNumberOfFrames(); ++i) {
----------------
krasin wrote:
> You may also consider to have a macro instead of a function, as it would allow to preserve the code of line. See for example: https://github.com/llvm-mirror/llvm/blob/358918e8dd375e41b4f51e7570a57e123b2951c6/unittests/Support/Path.cpp#L31
> 
> The macro in the example is gtest-specific, but it still gives an idea.
This doesn't seem to be the way things are done in llvm tools. I'd keep it as is if you don't mind.


================
Comment at: tools/sancov/sancov.cc:267
@@ +266,3 @@
+  for (const auto Section : O.sections()) {
+    if (Section.isVirtual() || !Section.isText())
+      continue;
----------------
I have no idea. This is how it is in objdump. Do you know why?

================
Comment at: tools/sancov/sancov.cc:347
@@ +346,3 @@
+  for (const auto &FnLoc : FnLocs)
+    FilePrefix = CommonPrefix(FilePrefix, FnLoc.Loc.FileName);
+
----------------
krasin wrote:
> What happens if you only have a single file? I have a feeling that the file name will be printed as an empty string.
It won't see the if (FileName.size() > FilePrefix.size()) below. 


http://reviews.llvm.org/D14889