Page MenuHomePhabricator

[NFC] Add missing doxygen file tag in llvm/include/llvm/ADT/ headers
ClosedPublic

Authored by xgupta on Dec 19 2021, 9:09 AM.

Details

Summary

Few header file don't have file tag in them. This patch can be help
in viewing doxygen documentation. When we hover on the included header
file, small description will display.

Diff Detail

Event Timeline

xgupta created this revision.Dec 19 2021, 9:09 AM
xgupta requested review of this revision.Dec 19 2021, 9:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 19 2021, 9:09 AM
xgupta updated this revision to Diff 395343.Dec 19 2021, 9:14 AM

minor fix after lint

Thanks for this! Generally LG, but I did have some questions.

llvm/include/llvm/ADT/BreadthFirstIterator.h
13–14

I'm not super familiar with Doxygen, so please forgive me in this is a silly question, but how well does Doxygen handle multiple paragraphs of text? I was assuming this would need some sort of delimiters for this situation.

llvm/include/llvm/ADT/DepthFirstIterator.h
15

I'm not certain what clang-format is after here -- can you investigate?

llvm/include/llvm/ADT/IntervalMap.h
32

Same here about clang-format.

llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
21

Does the code block need additional Doxygen markup?

llvm/include/llvm/ADT/Waymarking.h
16–19 ↗(On Diff #395456)

Just to double-check -- have you verified that ASCII art like in this comment is rendered fine in the Doxygen output?

xgupta updated this revision to Diff 397052.Jan 3 2022, 6:11 AM
xgupta marked 5 inline comments as done.

clang-format

xgupta added a comment.EditedJan 3 2022, 6:11 AM

Thanks for review.

Edit -

I'm not super familiar with Doxygen, so please forgive me in this is a silly question, but how well does Doxygen handle multiple paragraphs of text? I was assuming this would need some sort of delimiters for this situation.

Attached a{F21411745} screenshot-

llvm/include/llvm/ADT/BreadthFirstIterator.h
13–14

I have uploaded a screenshot to show how it looks when we hover on this included file.

llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
21

When hover it shows - "This file defines the RefCountedBase, ThreadSafeRefCountedBase, and IntrusiveRefCntPtr classes."

Does the code block need additional Doxygen markup?

I am also not sure.

llvm/include/llvm/ADT/Waymarking.h
16–19 ↗(On Diff #395456)

Sorry, It is not included in any file where I can check output in a .html file in a browser.

xgupta@archlinux ~/llvm/llvm-project (main?) $grep -nr "Waymarking.h" . --exclude-dir=build
./clang/docs/tools/clang-formatted-files.txt:4516:llvm/include/llvm/ADT/Waymarking.h
./llvm/unittests/ADT/WaymarkingTest.cpp:9:#include "llvm/ADT/Waymarking.h"
./llvm/include/llvm/ADT/Waymarking.h:1://===- Waymarking.h - Array waymarking algorithm ----------------*- C++ -*-===//
xgupta added a comment.Jan 3 2022, 6:29 AM

Thanks for review.

llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
21

Sorry, It needs markup changes, I will update soon.

llvm/include/llvm/ADT/Waymarking.h
16–19 ↗(On Diff #395456)

Sorry, I didn't check it, I will update after some time. It is not rendered correctly as seen in this screenshot.

xgupta updated this revision to Diff 403280.Jan 26 2022, 8:19 AM

Address comments

xgupta accepted this revision.EditedJan 28 2022, 6:12 AM

@aaron.ballman should I commit the patch? I have checked the html files in browser they looks good now.

Edit - Don't want to accept it myself.

This revision is now accepted and ready to land.Jan 28 2022, 6:12 AM
aaron.ballman accepted this revision.Jan 28 2022, 6:22 AM

LGTM assuming the ASCII art is rendering properly (though, given that the file is unused, I'm half tempted to pull the file out of the repo entirely).

llvm/include/llvm/ADT/Waymarking.h
16–19 ↗(On Diff #395456)

Is this now rendering properly?

Thank you for review.

llvm/include/llvm/ADT/Waymarking.h
16–19 ↗(On Diff #395456)

Yes. See -

Thank you for review.

Thank you for pinging me on it!

llvm/include/llvm/ADT/Waymarking.h
16–19 ↗(On Diff #395456)

Awesome! FWIW, that we had to think about this at all spurred me to create https://reviews.llvm.org/D118467.

aaron.ballman added inline comments.Jan 28 2022, 11:27 AM
llvm/include/llvm/ADT/Waymarking.h
16–19 ↗(On Diff #395456)

Just to warn you, that review above landed, so you'll get merge conflicts with your changes here. No need to have additional review for them though, you're free to land whenever.

This revision was landed with ongoing or failed builds.Jan 28 2022, 10:57 PM
This revision was automatically updated to reflect the committed changes.