I'm getting a slight code smell here that we are slowly building towards a full C++ specification API.
Prefer accepting const Twine & instead of std::string. It accepts many more parameter types (e.g. StringRef, std::string, llvm::formatv, etc.), and can easily be converted to string.
Not sure this comment is true given that we have isPublic below.
Missing comment here?
nit: Drop the const, we don't generally add them.
Also please document these fields.
This needs some documentation.
nit: I'd use ArrayRef here instead.
Can we just put the vector on the class and return ArrayRef instead?
This needs some documentation.
I agree. I'm not necessarily happy about it, but it absolutely is more maintainable than writing to a naked raw_ostream.
Good point, but what's your opinion on the stringify function I added? I wanted to elide copying std::string when it's passed in directly, and I've had some issues with const Twine &; for example,
void one(const Twine &value = ""); void one(bool value = false);
Confusingly, one("C string") calls one(bool)
LLVM should have functionality for this: https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ADT/BitmaskEnum.h
Which methods do we have that takes bool or Twine? If it isn't a lot of methods, we can always just add an overload specialized for const char */StringRef.
It's giving me an off feeling, but that may be that we generally try not to use std::string directly.
AFAIR we rely on the compiler to warn/error about missing cases.
Please switch this to an if/else to make it easier to read.
Please use proper foreach loops for these, the llvm::for_each doesn't add much.
I'd just use a proper foreach loop here, using llvm::for_each doesn't really add much.
semi unrelated, but the name "reindent" is throwing me off. It doesn't read like something that is emitted to the stream, I keep expecting it to return a reindented string. Can we rename that to something better?
raw_string_ostream would likely be cleaner here.
Drop the consts, we don't really add them unless really necessary (i.e. if they add value).
Can you document what this does? The doc on the other finalize was quite nice.
Same comment about () here and elsewhere.
Can we drop the ()?
Can we drop the ()? We generally don't wrap unless necessary.
Is this right, AFAICT Method::Declaration & declaration is always false.
I went for the stringify route because occasionally in ODS we create std::string and maybe something takes ownership of it. I got tired of debugging segmentation faults because .str() wasn't being called somewhere and a StringRef or Twine or format object was outliving its value.
I didn't know about this! But also, it doesn't seem to work because the functions need to be constexpr (and there are asserts in them...)
I agree. I initially thought the same.
This actually expands to declaration ? Method::Declaration : Method::None. If that's not intuitive, I can remove it.
Please remove this, the code that this creates is not readable.
I see now, you've defined a custom operator& for that. Please remove that, it creates a hacky difference in behavior that is not what someone what naturally expect.
In general, please avoid overloading operators like this. It creates code that is difficult to read/maintain just for the sake of saving a line or two. Prefer writing clean/readable code over "tricky" code.