[Lldb-commits] [PATCH] D49963: Preliminary patch to support prompt interpolation

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

This will be very cool. Biggest issues to watch out for is to make sure it doesn't return incorrect values when running for things like the thread count. If you switch to use the "m_debugger.GetCommandInterpreter().GetExecutionContext()" it should solve this by making sure the frame and thread are not filled in when the process is running.  It might also be racy. For example if you say "continue", hopefully the process will be resumed by the time the prompt is asked to refresh itself. Since we get events asynchronously this might be a problem.



================
Comment at: include/lldb/Core/FormatEntity.h:86
       FrameIndex,
+      FrameIndex1Based,
+      FrameCount,
----------------
Rename to FrameIndexID. It is similar to the thread index ID which is one based.


================
Comment at: include/lldb/Core/IOHandler.h:456
+private:
+  std::string m_prompt_string;
 };
----------------
If this will only contain a prompt that is a format string, you should store the compiled format so we don't have to parse it each time the prompt is displayed. Maybe this should be:

```
  FormatEntity::Entry m_prompt_format;
```



================
Comment at: include/lldb/Host/Editline.h:93
 typedef std::shared_ptr<EditlineHistory> EditlineHistorySP;
-
+  
 typedef bool (*IsInputCompleteCallbackType)(Editline *editline,
----------------
Remove whitespace


================
Comment at: source/Core/FormatEntity.cpp:125
     ENTRY("index", FrameIndex, UInt32),
+    ENTRY("index1based", FrameIndex1Based, UInt32),
+    ENTRY("count", FrameCount, UInt32),
----------------
rename to "index_id" and fix enum to FrameIndexID


================
Comment at: source/Core/FormatEntity.cpp:357
     ENUM_TO_CSTR(FrameIndex);
+    ENUM_TO_CSTR(FrameIndex1Based);
+    ENUM_TO_CSTR(FrameCount);
----------------
FrameIndexID


================
Comment at: source/Core/FormatEntity.cpp:1443
 
+  case Entry::Type::FrameIndex1Based:
+    if (exe_ctx) {
----------------
FrameIndexID


================
Comment at: source/Interpreter/PromptInterpolation.cpp:1
+//===-- PromptInterpolation.cpp ------------------------------------*- C++ -*-===//
+//
----------------
Not sure this file is needed?


================
Comment at: source/Interpreter/PromptInterpolation.cpp:33-46
+  TargetSP target = m_debugger.GetSelectedTarget();
+
+  if (!target) {
+    interpolated_prompt << kFallbackPrompt;
+    return interpolated_prompt.str();
+  }
+
----------------
Remove all of this and just call:

```
  ExecutionContext exe_ctx = m_debugger.GetCommandInterpreter().GetExecutionContext();
```

It will do the right thing and leave the thread and frame out of the context if we are running. There is no need to fallback to using kFallbackPrompt. You can make your prompt hardened against values not being available. Lets say the thread count isn't available: you can just put extra {} around the string that contains the thread count like:

```
"{num threads = ${thread.count}}"
```
If any variable inside the extra scope is not available, it will be left out. So there should be no need to use a fallback if the prompt is made correctly. 




================
Comment at: source/Interpreter/PromptInterpolation.cpp:50
+  FormatEntity::Entry root;
+  Status parse_status = FormatEntity::Parse(prompt_format, root);
+  bool formatted = false;
----------------
This should be parsed once and kept around. Not parsed every time the prompt is displayed. 


Repository:
  rL LLVM

https://reviews.llvm.org/D49963